From cd8e78a1750a70e86b90816473edbb6cf65e890b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 12 Sep 2022 06:44:08 +0200 Subject: [PATCH 1/9] Fix invalid addressing mode scale in debug mode --- src/coreclr/jit/codegencommon.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 45cbd8ac6c0737..4fbce276ed9b58 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1387,7 +1387,7 @@ bool CodeGen::genCreateAddrMode( if (fold) { - ssize_t tmpMul; + ssize_t tmpMul = 1; GenTree* index; if ((rv2->gtOper == GT_MUL || rv2->gtOper == GT_LSH) && (rv2->AsOp()->gtOp2->IsCnsIntOrI())) @@ -1417,10 +1417,7 @@ bool CodeGen::genCreateAddrMode( ssize_t ixv = index->AsIntConCommon()->IconValue(); /* Scale the index if necessary */ - if (tmpMul) - { - ixv *= tmpMul; - } + ixv *= tmpMul; if (FitsIn(cns + ixv)) { From 390d1f05b36604565f8c37d688e1828faeef2035 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 12 Sep 2022 06:48:06 +0200 Subject: [PATCH 2/9] Add a test --- .../JitBlue/Runtime_75312/Runtime_75312.il | 48 +++++++++++++++++++ .../Runtime_75312/Runtime_75312.ilproj | 8 ++++ 2 files changed, 56 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il new file mode 100644 index 00000000000000..665abcfeeb4f5f --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il @@ -0,0 +1,48 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern System.Runtime {} +.assembly extern System.Runtime.Extensions {} +.assembly extern System.Console {} +.assembly Runtime_75312 {} + +.class public abstract auto ansi sealed beforefieldinit Runtime_75312 + extends [System.Runtime]System.Object +{ + .method private hidebysig static + int32 Main () cil managed + { + .entrypoint + .maxstack 2 + .locals init ( + [0] int64 a + ) + ldc.i8 1234605616436508552 + stloc.0 + ldc.i4 1146447579 + ldloca.s 0 + conv.u + newobj instance void [System.Runtime]System.IntPtr::.ctor(void*) + call int32 [System.Runtime]System.IntPtr::op_Explicit(native int) + call int32 Runtime_75312::Test(int32) + sub + ret + } + + .method private hidebysig static int32 Test (int32 lcl) cil managed noinlining nooptimization + { + .maxstack 8 + ldarg.0 + ldc.i4.3 + ldc.i4.0 + mul + ldc.i4.2 + shl + add + ldc.i4.1 + add + conv.i + ldind.i4 + ret + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj new file mode 100644 index 00000000000000..be0bca337e251c --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj @@ -0,0 +1,8 @@ + + + Exe + + + + + From 09714b02d064218e4f1b1384c77747a30956db12 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 12 Sep 2022 07:01:07 +0200 Subject: [PATCH 3/9] Update Runtime_75312.il --- .../JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il index 665abcfeeb4f5f..ef3f9b67f82934 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.il @@ -32,6 +32,10 @@ .method private hidebysig static int32 Test (int32 lcl) cil managed noinlining nooptimization { .maxstack 8 + + // return *(int*)(arg0 + ((3 * 0) << 2) + 1); + // to avoid constant folding in Roslyn (even for Debug) it's written in IL + ldarg.0 ldc.i4.3 ldc.i4.0 From 4e30ed30343d493d14b3f8963903093f285544ea Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 12 Sep 2022 08:19:23 +0200 Subject: [PATCH 4/9] Update Runtime_75312.ilproj --- .../JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj index be0bca337e251c..5e9fc16ea3a67c 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75312/Runtime_75312.ilproj @@ -1,6 +1,9 @@ Exe + true + None + True From 1c18bb029cff9edc1f0e91d0efe36628d9887b1f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 12 Sep 2022 21:36:35 +0200 Subject: [PATCH 5/9] Drop unneeded initialization --- src/coreclr/jit/codegencommon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 4fbce276ed9b58..54f1e02d8de8fe 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1387,7 +1387,7 @@ bool CodeGen::genCreateAddrMode( if (fold) { - ssize_t tmpMul = 1; + ssize_t tmpMul; GenTree* index; if ((rv2->gtOper == GT_MUL || rv2->gtOper == GT_LSH) && (rv2->AsOp()->gtOp2->IsCnsIntOrI())) From e3f5dd0ee16548d295e3a434f9a14820a9664495 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 12 Sep 2022 22:24:09 +0200 Subject: [PATCH 6/9] Slightly better fix --- src/coreclr/jit/codegencommon.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 54f1e02d8de8fe..b09f9169cb868e 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1389,6 +1389,7 @@ bool CodeGen::genCreateAddrMode( { ssize_t tmpMul; GenTree* index; + bool indexIsEffectivelyZero = false; if ((rv2->gtOper == GT_MUL || rv2->gtOper == GT_LSH) && (rv2->AsOp()->gtOp2->IsCnsIntOrI())) { @@ -1401,6 +1402,12 @@ bool CodeGen::genCreateAddrMode( { tmpMul *= mul; } + + if (tmpMul == 0) + { + // "Index * 0" (if it wasn't folded earlier) means the index is zero + indexIsEffectivelyZero = true; + } } else { @@ -1410,14 +1417,22 @@ bool CodeGen::genCreateAddrMode( tmpMul = mul; } + if (indexIsEffectivelyZero) + { + mul = 0; + rv2 = nullptr; + } /* Get hold of the array index and see if it's a constant */ - if (index->IsIntCnsFitsInI32()) + else if (index->IsIntCnsFitsInI32()) { /* Get hold of the index value */ ssize_t ixv = index->AsIntConCommon()->IconValue(); /* Scale the index if necessary */ - ixv *= tmpMul; + if (tmpMul) + { + ixv *= tmpMul; + } if (FitsIn(cns + ixv)) { From 219beba478e91799866e121cdd18205f9bd52e20 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 12 Sep 2022 23:51:25 +0200 Subject: [PATCH 7/9] Test an even better fix? --- src/coreclr/jit/codegencommon.cpp | 66 ------------------------------- 1 file changed, 66 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index b09f9169cb868e..21fc8ffd5ef458 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1371,7 +1371,6 @@ bool CodeGen::genCreateAddrMode( if (rv2) { /* Make sure a GC address doesn't end up in 'rv2' */ - if (varTypeIsGC(rv2->TypeGet())) { noway_assert(rv1 && !varTypeIsGC(rv1->TypeGet())); @@ -1379,73 +1378,8 @@ bool CodeGen::genCreateAddrMode( tmp = rv1; rv1 = rv2; rv2 = tmp; - rev = !rev; } - - /* Special case: constant array index (that is range-checked) */ - - if (fold) - { - ssize_t tmpMul; - GenTree* index; - bool indexIsEffectivelyZero = false; - - if ((rv2->gtOper == GT_MUL || rv2->gtOper == GT_LSH) && (rv2->AsOp()->gtOp2->IsCnsIntOrI())) - { - /* For valuetype arrays where we can't use the scaled address - mode, rv2 will point to the scaled index. So we have to do - more work */ - - tmpMul = compiler->optGetArrayRefScaleAndIndex(rv2, &index DEBUGARG(false)); - if (mul) - { - tmpMul *= mul; - } - - if (tmpMul == 0) - { - // "Index * 0" (if it wasn't folded earlier) means the index is zero - indexIsEffectivelyZero = true; - } - } - else - { - /* May be a simple array. rv2 will points to the actual index */ - - index = rv2; - tmpMul = mul; - } - - if (indexIsEffectivelyZero) - { - mul = 0; - rv2 = nullptr; - } - /* Get hold of the array index and see if it's a constant */ - else if (index->IsIntCnsFitsInI32()) - { - /* Get hold of the index value */ - ssize_t ixv = index->AsIntConCommon()->IconValue(); - - /* Scale the index if necessary */ - if (tmpMul) - { - ixv *= tmpMul; - } - - if (FitsIn(cns + ixv)) - { - /* Add the scaled index to the offset value */ - - cns += ixv; - - /* There is no scaled operand any more */ - mul = 0; - rv2 = nullptr; - } - } - } } // We shouldn't have [rv2*1 + cns] - this is equivalent to [rv1 + cns] From 5e6923edf307ff998715b4d2e6a94d99591dcde1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Sep 2022 02:02:32 +0200 Subject: [PATCH 8/9] Refactoring --- src/coreclr/jit/codegencommon.cpp | 48 +++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 21fc8ffd5ef458..209883e9e3887b 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1370,16 +1370,54 @@ bool CodeGen::genCreateAddrMode( if (rv2) { - /* Make sure a GC address doesn't end up in 'rv2' */ + // Make sure a GC address doesn't end up in 'rv2' if (varTypeIsGC(rv2->TypeGet())) { noway_assert(rv1 && !varTypeIsGC(rv1->TypeGet())); - - tmp = rv1; - rv1 = rv2; - rv2 = tmp; + std::swap(rv1, rv2); rev = !rev; } + + // Special case: constant array index (that is range-checked) + if (fold && rv2->OperIs(GT_MUL, GT_LSH) && (rv2->gtGetOp2()->IsCnsIntOrI())) + { + // For valuetype arrays where we can't use the scaled address + // mode, rv2 will point to the scaled index. So we have to do + // more work + GenTree* index; + ssize_t indexScale = compiler->optGetArrayRefScaleAndIndex(rv2, &index DEBUGARG(false)); + + // Apply accumulated scale if it exists + if (mul) + { + indexScale *= mul; + } + + // "index * 0" means index is zero + if (indexScale == 0) + { + mul = 0; + rv2 = nullptr; + } + else if (index->IsIntCnsFitsInI32()) + { + ssize_t constantIndex = index->AsIntConCommon()->IconValue() * indexScale; + if (constantIndex == 0) + { + // while scale is a non-zero constant, the actual index is zero so drop it + mul = 0; + rv2 = nullptr; + } + else if (FitsIn(cns + constantIndex)) + { + // Add the constant index to the accumulated offset value + cns += constantIndex; + // and get rid of index + mul = 0; + rv2 = nullptr; + } + } + } } // We shouldn't have [rv2*1 + cns] - this is equivalent to [rv1 + cns] From caf0d40e047b3065bc3235bf0a2b48587ff24efe Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Sep 2022 15:39:33 +0200 Subject: [PATCH 9/9] address feedback --- src/coreclr/jit/codegencommon.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 209883e9e3887b..7278506f86cf6d 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1379,18 +1379,15 @@ bool CodeGen::genCreateAddrMode( } // Special case: constant array index (that is range-checked) - if (fold && rv2->OperIs(GT_MUL, GT_LSH) && (rv2->gtGetOp2()->IsCnsIntOrI())) + if (fold) { - // For valuetype arrays where we can't use the scaled address - // mode, rv2 will point to the scaled index. So we have to do - // more work - GenTree* index; - ssize_t indexScale = compiler->optGetArrayRefScaleAndIndex(rv2, &index DEBUGARG(false)); + // By default, assume index is rv2 and indexScale is mul (or 1 if mul is zero) + GenTree* index = rv2; + ssize_t indexScale = mul == 0 ? 1 : mul; - // Apply accumulated scale if it exists - if (mul) + if (rv2->OperIs(GT_MUL, GT_LSH) && (rv2->gtGetOp2()->IsCnsIntOrI())) { - indexScale *= mul; + indexScale *= compiler->optGetArrayRefScaleAndIndex(rv2, &index DEBUGARG(false)); } // "index * 0" means index is zero