From c76ebe999dd1e9cac28040f98c837e116f772053 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 15 Aug 2021 23:36:20 +0300 Subject: [PATCH 1/2] Add the test --- .../FieldListByteNodeTypeMismatchX86.cs | 75 +++++++++++++++++++ .../FieldListByteNodeTypeMismatchX86.csproj | 13 ++++ 2 files changed, 88 insertions(+) create mode 100644 src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.cs create mode 100644 src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.csproj diff --git a/src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.cs b/src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.cs new file mode 100644 index 0000000000000..1872ca38d7e62 --- /dev/null +++ b/src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.cs @@ -0,0 +1,75 @@ +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +public unsafe class FieldListByteNodeTypeMismatchX86 +{ + private static readonly byte* _addr = (byte*)Marshal.AllocHGlobal(20); + private static int _result = 100; + + public static int Main() + { + int sum = 0; + Problem(&sum); + + return _result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Problem(int* sum) + { + // Just a loop with some computations so that we can use + // callee-saved registers (which happen to be non-byteable ones). + for (int i = 0; i < 10; i++) + { + var i1 = i ^ i; + var i2 = i | i; + var i3 = i & i; + var i4 = i1 + i + i2 - i3; + i4 = i2 ^ i4; + + *sum += i4; + } + + Sx2x1 s; + byte* j = Addr(); + + int o1 = j[-2]; + int o2 = j[-1]; + s.Short = j[0]; + s.Byte = j[1]; + + o1 = o1 + o1; + o2 = o2 + o2; + + if (s.Byte != 1) + { + return; + } + + // Here assertion propagation will make s.Byte into CNS_INT(TYP_INT), yet the RA must + // still allocate a byteable register so that it can be saved to the stack correctly. + Call(s, 0, 0, o2, o1, j); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte* Addr() + { + _addr[11] = 1; + return _addr + 10; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Call(Sx2x1 s, int i3, int i4, int i5, int i6, byte* i1) + { + if (s.Byte != 1) + { + _result = -1; + } + } + + private struct Sx2x1 + { + public short Short; + public byte Byte; + } +} diff --git a/src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.csproj b/src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.csproj new file mode 100644 index 0000000000000..6d06d331a2a59 --- /dev/null +++ b/src/tests/JIT/Directed/StructABI/FieldListByteNodeTypeMismatchX86.csproj @@ -0,0 +1,13 @@ + + + Exe + true + + + None + True + + + + + From b2a8d231ee02af65715f8ffa0ff193c2ed160ddd Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 15 Aug 2021 23:42:07 +0300 Subject: [PATCH 2/2] Fix the bug Assertion propagation can replace a TYP_UBYTE field in a field list with a TYP_INT constant, so LSRA must use the actual field type when deciding whether an internal register to allocate must be byteable. --- src/coreclr/jit/lowerxarch.cpp | 8 ++++---- src/coreclr/jit/lsraxarch.cpp | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 43c0df6204236..38a7655da7988 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -452,10 +452,10 @@ void Lowering::LowerPutArgStk(GenTreePutArgStk* putArgStk) unsigned prevOffset = putArgStk->GetStackByteSize(); for (GenTreeFieldList::Use& use : fieldList->Uses()) { - GenTree* const fieldNode = use.GetNode(); - const var_types fieldType = fieldNode->TypeGet(); - const unsigned fieldOffset = use.GetOffset(); - assert(fieldType != TYP_LONG); + GenTree* const fieldNode = use.GetNode(); + const unsigned fieldOffset = use.GetOffset(); + + assert(!fieldNode->TypeIs(TYP_LONG)); // We can treat as a slot any field that is stored at a slot boundary, where the previous // field is not in the same slot. (Note that we store the fields in reverse order.) diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp index 854e4521ec900..4b9a7a43cfca4 100644 --- a/src/coreclr/jit/lsraxarch.cpp +++ b/src/coreclr/jit/lsraxarch.cpp @@ -1501,18 +1501,18 @@ int LinearScan::BuildPutArgStk(GenTreePutArgStk* putArgStk) for (GenTreeFieldList::Use& use : putArgStk->gtOp1->AsFieldList()->Uses()) { GenTree* const fieldNode = use.GetNode(); - const var_types fieldType = fieldNode->TypeGet(); const unsigned fieldOffset = use.GetOffset(); + const var_types fieldType = use.GetType(); #ifdef TARGET_X86 assert(fieldType != TYP_LONG); #endif // TARGET_X86 #if defined(FEATURE_SIMD) - // Note that we need to check the GT_FIELD_LIST type, not 'fieldType'. This is because the - // GT_FIELD_LIST will be TYP_SIMD12 whereas the fieldType might be TYP_SIMD16 for lclVar, where + // Note that we need to check the field type, not the type of the node. This is because the + // field type will be TYP_SIMD12 whereas the node type might be TYP_SIMD16 for lclVar, where // we "round up" to 16. - if ((use.GetType() == TYP_SIMD12) && (simdTemp == nullptr)) + if ((fieldType == TYP_SIMD12) && (simdTemp == nullptr)) { simdTemp = buildInternalFloatRegisterDefForNode(putArgStk); }