Code review feedback

* Move architecture filtering for Hfas to `MetadataFieldLayoutAlgorithm` * Switch `GCPointerMap` to use `uint` for flags instead of `int`
dotnet · Apr 20, 2020 · 35bf836 · 35bf836
1 parent 2d3e4b6
commit 35bf836
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 16 deletions.
diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
@@ -843,6 +843,9 @@ private ValueTypeShapeCharacteristics ComputeHomogeneousFloatAggregateCharacteri
 
             MetadataType metadataType = (MetadataType)type;
 
+            if (type.Context.Target.Architecture != TargetArchitecture.ARM && type.Context.Target.Architecture != TargetArchitecture.ARM64)
+                return ValueTypeShapeCharacteristics.None;
+
             // No HFAs with explicit layout. There may be cases where explicit layout may be still
             // eligible for HFA, but it is hard to tell the real intent. Make it simple and just 
             // unconditionally disable HFAs for explicit layout.

diff --git a/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs b/src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs
@@ -16,7 +16,7 @@ namespace Internal.TypeSystem
     public partial struct GCPointerMap : IEquatable<GCPointerMap>, IComparable<GCPointerMap>
     {
         // Each bit in this array represents a pointer-sized cell.
-        private int[] _gcFlags;
+        private uint[] _gcFlags;
 
         private int _numCells;
 
@@ -86,7 +86,7 @@ public bool this[int index]
             }
         }
 
-        public GCPointerMap(int[] gcFlags, int numCells)
+        public GCPointerMap(uint[] gcFlags, int numCells)
         {
             Debug.Assert(numCells <= gcFlags.Length << 5);
             _gcFlags = gcFlags;
@@ -119,7 +119,7 @@ public override int GetHashCode()
         {
             int hashCode = 0;
             for (int i = 0; i < _gcFlags.Length; i++)
-                hashCode ^= _gcFlags[i];
+                hashCode ^= (int)_gcFlags[i];
             return hashCode;
         }
 
@@ -139,7 +139,7 @@ public int CompareTo(GCPointerMap other)
             for (int i = 0; i < _gcFlags.Length; i++)
             {
                 if (_gcFlags[i] != other._gcFlags[i])
-                    return _gcFlags[i] - other._gcFlags[i];
+                    return (int)(_gcFlags[i] - other._gcFlags[i]);
             }
 
             Debug.Assert(Equals(other));
@@ -154,7 +154,7 @@ public struct GCPointerMapBuilder
     {
         // Each bit in this array represents a pointer-sized cell.
         // Bits start at the least significant bit.
-        private int[] _gcFlags;
+        private uint[] _gcFlags;
 
         private int _pointerSize;
 
@@ -172,12 +172,12 @@ public GCPointerMapBuilder(int numBytes, int pointerSize)
             {
                 // Given the number of cells, how many Int32's do we need to represent them?
                 // (It's one bit per cell, but this time we need to round up.)
-                _gcFlags = new int[((numPointerSizedCells - 1) >> 5) + 1];
+                _gcFlags = new uint[((numPointerSizedCells - 1) >> 5) + 1];
             }
             else
             {
                 // Not big enough to fit even a single pointer.
-                _gcFlags = Array.Empty<int>();
+                _gcFlags = Array.Empty<uint>();
             }
 
             _pointerSize = pointerSize;
@@ -197,7 +197,7 @@ public void MarkGCPointer(int offset)
 
             int cellIndex = absoluteOffset / _pointerSize;
 
-            _gcFlags[cellIndex >> 5] |= 1 << (cellIndex & 0x1F);
+            _gcFlags[cellIndex >> 5] |= 1u << (cellIndex & 0x1F);
         }
 
         public GCPointerMapBuilder GetInnerBuilder(int offset, int size)
@@ -241,11 +241,11 @@ public override string ToString()
 
     public struct BitEnumerator
     {
-        private int[] _buffer;
+        private uint[] _buffer;
         private int _limitBit;
         private int _currentBit;
 
-        public BitEnumerator(int[] buffer, int startBit, int numBits)
+        public BitEnumerator(uint[] buffer, int startBit, int numBits)
         {
             Debug.Assert(startBit >= 0 && numBits >= 0);
             Debug.Assert(startBit + numBits <= buffer.Length << 5);

diff --git a/...ssgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs b/...ssgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
@@ -71,24 +71,23 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ
                 flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout_Empty;
             }
 
-            bool isHfa = defType.IsHfa && defType.Context.Target.Architecture == TargetArchitecture.ARM || defType.Context.Target.Architecture == TargetArchitecture.ARM64;
-            if (isHfa)
+            if (defType.IsHfa)
             {
                 flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_HFA;
             }
 
             dataBuilder.EmitUInt((uint)flags);
             dataBuilder.EmitUInt((uint)size);
 
-            if (isHfa)
+            if (defType.IsHfa)
             {
                 switch (defType.HfaElementType.Category)
                 {
                     case TypeFlags.Single:
                         dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R4);
                         break;
                     case TypeFlags.Double:
-                        dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R4);
+                        dataBuilder.EmitUInt((uint)CorElementType.ELEMENT_TYPE_R8);
                         break;
                 }
             }
@@ -109,7 +108,7 @@ private static void EncodeTypeLayout(ObjectDataSignatureBuilder dataBuilder, Typ
                 {
                     if (bit)
                     {
-                        encodedGCRefMap[bitIndex / 8] += (byte)(1 << (bitIndex & 7));
+                        encodedGCRefMap[bitIndex / 8] |= (byte)(1 << (bitIndex & 7));
                     }
 
                     ++bitIndex;
@@ -126,7 +125,7 @@ private static int GetClassAlignmentRequirement(MetadataType type)
         {
             int alignment = type.Context.Target.PointerSize;
 
-            if (((MetadataType)type).HasLayout())
+            if (type.HasLayout())
             {
                 if (type.IsSequentialLayout || MarshalUtils.IsBlittableType(type))
                 {