From fd11209c0a4cad9499c7342a1a89d77ce9cf6e19 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 5 Sep 2022 10:08:35 -0700 Subject: [PATCH] Use the register of CAST for contained index operator (#74275) * Rename test files * Fix index scale for cast * Do not contain index if indir is not containable. * Revert "Do not contain index if indir is not containable." This reverts commit e79d17d92ceb0eed5ae1bfd03c2d1d6b171ab17f. * Instead try to contain the LEA * IsSafeToContainMem() check * Do IsSafeToContainMem() only if needed * Add test case * Fix merge error * fix the test case * review comment --- src/coreclr/jit/codegenarmarch.cpp | 24 +++++++-- src/coreclr/jit/lower.cpp | 52 ++++++++++++------- .../JitBlue/Runtime_74117/Runtime_74117.cs | 27 ++++++++++ .../Runtime_74117.csproj} | 0 .../{Runtime_73821.cs => Runtime_74373.cs} | 0 .../Runtime_74373/Runtime_74373.csproj | 10 ++++ 6 files changed, 89 insertions(+), 24 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.cs rename src/tests/JIT/Regression/JitBlue/{Runtime_74373/Runtime_73821.csproj => Runtime_74117/Runtime_74117.csproj} (100%) rename src/tests/JIT/Regression/JitBlue/Runtime_74373/{Runtime_73821.cs => Runtime_74373.cs} (100%) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.csproj diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 0702ee2f737fb..f7bb7a91f14e2 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -4458,12 +4458,26 @@ 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 + { + // 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 diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 21ec29cfaeb6c..d2d82916b6f0b 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5362,29 +5362,43 @@ 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()); + if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType)) + { + 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); + } + } + 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(); + 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); + // '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)) + { + // 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); + } + } } } #endif 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..0bb8fa2d83f72 --- /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, 0); + 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_74373/Runtime_73821.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.csproj similarity index 100% rename from src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_73821.csproj rename to src/tests/JIT/Regression/JitBlue/Runtime_74117/Runtime_74117.csproj 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_74373.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.csproj new file mode 100644 index 0000000000000..cf94135633b19 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_74373.csproj @@ -0,0 +1,10 @@ + + + Exe + True + true + + + + + \ No newline at end of file