From 7dd7403f02986009817ce7a9d200023270c0c4b5 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Aug 2022 07:50:57 -0700 Subject: [PATCH 01/11] Rename test files --- .../JitBlue/Runtime_74373/{Runtime_73821.cs => Runtime_74373.cs} | 0 .../Runtime_74373/{Runtime_73821.csproj => Runtime_74373.csproj} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename src/tests/JIT/Regression/JitBlue/Runtime_74373/{Runtime_73821.cs => Runtime_74373.cs} (100%) rename src/tests/JIT/Regression/JitBlue/Runtime_74373/{Runtime_73821.csproj => Runtime_74373.csproj} (100%) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_73821.cs b/src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.cs similarity index 100% rename from src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_73821.cs rename to src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.cs diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_73821.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.csproj similarity index 100% rename from src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_73821.csproj rename to src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.csproj From 5a06ce65abf9c8bf0f9cb1ca0817418b0d420d4a Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 19 Aug 2022 09:29:10 -0700 Subject: [PATCH 02/11] Fix index scale for cast --- src/coreclr/jit/codegenarmarch.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 0702ee2f737fb..6a0dfc16dbb99 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -4458,12 +4458,24 @@ void CodeGen::genLeaInstruction(GenTreeAddrMode* lea) else { #ifdef TARGET_ARM64 - // Handle LEA with "contained" BFIZ - if (index->isContained() && index->OperIs(GT_BFIZ)) + + if (index->isContained()) { - assert(scale == 0); - scale = (DWORD)index->gtGetOp2()->AsIntConCommon()->IconValue(); - index = index->gtGetOp1()->gtGetOp1(); + if (index->OperIs(GT_BFIZ)) + { + // Handle LEA with "contained" BFIZ + assert(scale == 0); + scale = (DWORD)index->gtGetOp2()->AsIntConCommon()->IconValue(); + index = index->gtGetOp1()->gtGetOp1(); + } + else if (index->OperIs(GT_CAST)) + { + index = index->AsCast()->gtGetOp1(); + } + else + { + assert(!"There are more unhandled nodes for contained index for ARM64"); + } } #endif From 592fafea4a1be0a01de38ece306c202a975bcebf Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 23 Aug 2022 11:00:53 -0700 Subject: [PATCH 03/11] Do not contain index if indir is not containable. --- src/coreclr/jit/lowerarmarch.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 9cab1d427ad1f..1e83bef4b8e01 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -1799,9 +1799,9 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode) GenTree* addr = indirNode->Addr(); - if ((addr->OperGet() == GT_LEA) && IsSafeToContainMem(indirNode, addr)) + if ((addr->OperGet() == GT_LEA)) { - bool makeContained = true; + bool makeContained = IsSafeToContainMem(indirNode, addr); #ifdef TARGET_ARM // ARM floating-point load/store doesn't support a form similar to integer @@ -1832,6 +1832,12 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode) { MakeSrcContained(indirNode, addr); } +#ifdef TARGET_ARM64 + else if (addr->AsAddrMode()->HasIndex()) + { + addr->AsAddrMode()->Index()->ClearContained(); + } +#endif } else if (addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR)) { From aae7ba5587210b4c3407793a3a69824085d1b0cf Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 23 Aug 2022 11:09:43 -0700 Subject: [PATCH 04/11] Revert "Do not contain index if indir is not containable." This reverts commit e79d17d92ceb0eed5ae1bfd03c2d1d6b171ab17f. --- src/coreclr/jit/lowerarmarch.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 1e83bef4b8e01..9cab1d427ad1f 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -1799,9 +1799,9 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode) GenTree* addr = indirNode->Addr(); - if ((addr->OperGet() == GT_LEA)) + if ((addr->OperGet() == GT_LEA) && IsSafeToContainMem(indirNode, addr)) { - bool makeContained = IsSafeToContainMem(indirNode, addr); + bool makeContained = true; #ifdef TARGET_ARM // ARM floating-point load/store doesn't support a form similar to integer @@ -1832,12 +1832,6 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode) { MakeSrcContained(indirNode, addr); } -#ifdef TARGET_ARM64 - else if (addr->AsAddrMode()->HasIndex()) - { - addr->AsAddrMode()->Index()->ClearContained(); - } -#endif } else if (addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR)) { From feb447621935113b741475f56fbe3776f908872e Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 23 Aug 2022 11:35:58 -0700 Subject: [PATCH 05/11] Instead try to contain the LEA --- src/coreclr/jit/lower.cpp | 45 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 3532c4e996866..8a7e6f9dcb6b6 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5362,30 +5362,37 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par } #ifdef TARGET_ARM64 - if ((index != nullptr) && index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) - { - index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. - MakeSrcContained(addrMode, index); - } - // Check if we can "contain" LEA(BFIZ) in order to extend 32bit index to 64bit as part of load/store. - if ((index != nullptr) && index->OperIs(GT_BFIZ) && index->gtGetOp1()->OperIs(GT_CAST) && - index->gtGetOp2()->IsCnsIntOrI() && !varTypeIsStruct(targetType)) + if (index != nullptr) { - // BFIZ node is a binary op where op1 is GT_CAST and op2 is GT_CNS_INT - GenTreeCast* cast = index->gtGetOp1()->AsCast(); - assert(cast->isContained()); - - const unsigned shiftBy = (unsigned)index->gtGetOp2()->AsIntCon()->IconValue(); - - // 'scale' and 'offset' have to be unset since we're going to use [base + index * SXTW/UXTW scale] form - // where there is no room for additional offsets/scales on ARM64. 'shiftBy' has to match target's width. - if (cast->CastOp()->TypeIs(TYP_INT) && cast->TypeIs(TYP_LONG) && (genTypeSize(targetType) == (1U << shiftBy)) && - (scale == 1) && (offset == 0)) + if (!IsSafeToContainMem(parent, index)) + { + index->ClearContained(); + } + else if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) { - // TODO: Make sure that genCreateAddrMode marks such BFIZ candidates as GTF_DONT_CSE for better CQ. + index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. MakeSrcContained(addrMode, index); } + else if (index->OperIs(GT_BFIZ) && index->gtGetOp1()->OperIs(GT_CAST) && index->gtGetOp2()->IsCnsIntOrI() && + !varTypeIsStruct(targetType)) + { + // Check if we can "contain" LEA(BFIZ) in order to extend 32bit index to 64bit as part of load/store. + // BFIZ node is a binary op where op1 is GT_CAST and op2 is GT_CNS_INT + GenTreeCast* cast = index->gtGetOp1()->AsCast(); + assert(cast->isContained()); + + const unsigned shiftBy = (unsigned)index->gtGetOp2()->AsIntCon()->IconValue(); + + // 'scale' and 'offset' have to be unset since we're going to use [base + index * SXTW/UXTW scale] form + // where there is no room for additional offsets/scales on ARM64. 'shiftBy' has to match target's width. + if (cast->CastOp()->TypeIs(TYP_INT) && cast->TypeIs(TYP_LONG) && + (genTypeSize(targetType) == (1U << shiftBy)) && (scale == 1) && (offset == 0)) + { + // TODO: Make sure that genCreateAddrMode marks such BFIZ candidates as GTF_DONT_CSE for better CQ. + MakeSrcContained(addrMode, index); + } + } } #endif From 42875bc55f0de98dfe0f892b73a03ae8c7f9b455 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 23 Aug 2022 11:42:42 -0700 Subject: [PATCH 06/11] IsSafeToContainMem() check --- src/coreclr/jit/lower.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 8a7e6f9dcb6b6..93437e5d05b73 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5363,13 +5363,11 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par #ifdef TARGET_ARM64 - if (index != nullptr) + // Check containment safety against the parent node - this will ensure that LEA with the contained + // index will itself always be contained. We do not support uncontained LEAs with contained indices. + if ((index != nullptr) && IsSafeToContainMem(parent, index)) { - if (!IsSafeToContainMem(parent, index)) - { - index->ClearContained(); - } - else if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) + if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) { index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. MakeSrcContained(addrMode, index); From 2416bda9c39195ac83e1f8398d1b385b9e7bf150 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Aug 2022 14:23:34 -0700 Subject: [PATCH 07/11] Do IsSafeToContainMem() only if needed --- src/coreclr/jit/lower.cpp | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 93437e5d05b73..c071327bd1ef2 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5363,14 +5363,18 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par #ifdef TARGET_ARM64 - // Check containment safety against the parent node - this will ensure that LEA with the contained - // index will itself always be contained. We do not support uncontained LEAs with contained indices. - if ((index != nullptr) && IsSafeToContainMem(parent, index)) + if (index != nullptr) { if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) { - index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. - MakeSrcContained(addrMode, index); + index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. + + if (IsSafeToContainMem(parent, index)) + { + // Check containment safety against the parent node - this will ensure that LEA with the contained + // index will itself always be contained. We do not support uncontained LEAs with contained indices. + MakeSrcContained(addrMode, index); + } } else if (index->OperIs(GT_BFIZ) && index->gtGetOp1()->OperIs(GT_CAST) && index->gtGetOp2()->IsCnsIntOrI() && !varTypeIsStruct(targetType)) @@ -5387,8 +5391,14 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par if (cast->CastOp()->TypeIs(TYP_INT) && cast->TypeIs(TYP_LONG) && (genTypeSize(targetType) == (1U << shiftBy)) && (scale == 1) && (offset == 0)) { - // TODO: Make sure that genCreateAddrMode marks such BFIZ candidates as GTF_DONT_CSE for better CQ. - MakeSrcContained(addrMode, index); + if (IsSafeToContainMem(parent, index)) + { + // Check containment safety against the parent node - this will ensure that LEA with the contained + // index will itself always be contained. We do not support uncontained LEAs with contained indices. + + // TODO: Make sure that genCreateAddrMode marks such BFIZ candidates as GTF_DONT_CSE for better CQ. + MakeSrcContained(addrMode, index); + } } } } From a6d59194f8f665f6c911ad1f37985ad13d6cba17 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Aug 2022 14:50:16 -0700 Subject: [PATCH 08/11] Add test case --- .../JitBlue/Runtime_74117/Runtime_74117.cs | 27 +++++++++++++++++++ .../Runtime_74117/Runtime_74117.csproj | 10 +++++++ 2 files changed, 37 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs new file mode 100644 index 0000000000000..9abd400ddc7d7 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs @@ -0,0 +1,27 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +unsafe class Runtime_74117 +{ + public unsafe static int Main(string[] args) + { + byte a = 5; + Problem(ref a, 5); + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static byte GetByte() => 1; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Problem(ref byte x, int a) + { + JitUse(&a); + Unsafe.Add(ref x, a) = GetByte(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void JitUse(T* arg) where T : unmanaged { } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.csproj new file mode 100644 index 0000000000000..cf94135633b19 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.csproj @@ -0,0 +1,10 @@ + + + Exe + True + true + + + + + \ No newline at end of file From 85c40cf6e70087d1b559ae031efe6bcfb63349f3 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Aug 2022 15:35:03 -0700 Subject: [PATCH 09/11] Fix merge error --- src/coreclr/jit/lower.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index c071327bd1ef2..37e35843030a2 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5367,12 +5367,11 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par { if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) { - index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. - if (IsSafeToContainMem(parent, index)) { // Check containment safety against the parent node - this will ensure that LEA with the contained // index will itself always be contained. We do not support uncontained LEAs with contained indices. + index->AsCast()->CastOp()->ClearContained(); // Uncontain any memory operands. MakeSrcContained(addrMode, index); } } From 512e922f1f60f5846b1ecdd5ec196962804dae4a Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 29 Aug 2022 11:56:45 -0700 Subject: [PATCH 10/11] fix the test case --- src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs index 9abd400ddc7d7..0bb8fa2d83f72 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs @@ -8,7 +8,7 @@ unsafe class Runtime_74117 public unsafe static int Main(string[] args) { byte a = 5; - Problem(ref a, 5); + Problem(ref a, 0); return 100; } From 0f14423fbc9dc19a6fb5e3d739bfc714146e27d1 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Sun, 4 Sep 2022 17:04:51 -0700 Subject: [PATCH 11/11] review comment --- src/coreclr/jit/codegenarmarch.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 6a0dfc16dbb99..f7bb7a91f14e2 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -4474,7 +4474,9 @@ void CodeGen::genLeaInstruction(GenTreeAddrMode* lea) } else { - assert(!"There are more unhandled nodes for contained index for ARM64"); + // Only BFIZ/CAST nodes should be present for for contained index on ARM64. + // If there are more, we need to handle them here. + unreached(); } } #endif