From 9c2d47bd87abc7c6835d7278b03cd4b7d6b90c27 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Wed, 7 Mar 2018 11:12:13 -0800 Subject: Mark operands of dead FIELD_LIST as unused This requires fixing the side-effects check in dead code elimination. Also, fixes gtSetFlags() to be usable from DCE in the non-legacy case. --- src/jit/gentree.cpp | 4 +- src/jit/lir.cpp | 5 + src/jit/liveness.cpp | 7 +- src/jit/lsraarm.cpp | 7 +- src/jit/lsraarm64.cpp | 7 +- src/jit/lsraxarch.cpp | 7 +- .../JitBlue/DevDiv_544983/DevDiv_544983.il | 151 +++++++++++++++++++++ .../JitBlue/DevDiv_544983/DevDiv_544983.ilproj | 23 ++++ 8 files changed, 204 insertions(+), 7 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.il create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.ilproj diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index 9b8f88bd85..937efef35b 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -8699,7 +8699,7 @@ bool GenTree::gtSetFlags() const // // Precondition we have a GTK_SMPOP // - if (!varTypeIsIntegralOrI(TypeGet())) + if (!varTypeIsIntegralOrI(TypeGet()) && (TypeGet() != TYP_VOID)) { return false; } @@ -8722,7 +8722,7 @@ bool GenTree::gtSetFlags() const } #else // !(defined(LEGACY_BACKEND) && !FEATURE_SET_FLAGS && defined(_TARGET_XARCH_)) -#if FEATURE_SET_FLAGS +#if FEATURE_SET_FLAGS && defined(LEGACY_BACKEND) assert(OperIsSimple()); #endif if (((gtFlags & GTF_SET_FLAGS) != 0) && (gtOper != GT_IND)) diff --git a/src/jit/lir.cpp b/src/jit/lir.cpp index a0a265d5ae..5a05e23c8e 100644 --- a/src/jit/lir.cpp +++ b/src/jit/lir.cpp @@ -1581,6 +1581,11 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const // Verify that the node is allowed in LIR. assert(node->IsLIR()); + // Some nodes should never be marked unused, as they must be contained in the backend. + // These may be marked as unused during dead code elimination traversal, but they *must* be subsequently + // removed. + assert(!node->IsUnusedValue() || !node->OperIs(GT_FIELD_LIST, GT_LIST, GT_INIT_VAL)); + // Verify that the REVERSE_OPS flag is not set. NOTE: if we ever decide to reuse the bit assigned to // GTF_REVERSE_OPS for an LIR-only flag we will need to move this check to the points at which we // insert nodes into an LIR range. diff --git a/src/jit/liveness.cpp b/src/jit/liveness.cpp index dac5a00e1b..67d9e827a9 100644 --- a/src/jit/liveness.cpp +++ b/src/jit/liveness.cpp @@ -2225,8 +2225,11 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR assert(!node->OperIsLocal()); if (!node->IsValue() || node->IsUnusedValue()) { - unsigned sideEffects = node->gtFlags & (GTF_SIDE_EFFECT | GTF_SET_FLAGS); - if ((sideEffects == 0) || ((sideEffects == GTF_EXCEPT) && !node->OperMayThrow(this))) + // We are only interested in avoiding the removal of nodes with direct side-effects + // (as opposed to side effects of their children). + // This default case should never include calls or assignments. + assert(!node->OperRequiresAsgFlag() && !node->OperIs(GT_CALL)); + if (!node->gtSetFlags() && !node->OperMayThrow(this)) { JITDUMP("Removing dead node:\n"); DISPNODE(node); diff --git a/src/jit/lsraarm.cpp b/src/jit/lsraarm.cpp index fb3df19139..df4a435589 100644 --- a/src/jit/lsraarm.cpp +++ b/src/jit/lsraarm.cpp @@ -373,8 +373,13 @@ void LinearScan::BuildNode(GenTree* tree) assert(info->srcCount == 2); break; - case GT_LIST: case GT_FIELD_LIST: + // These should always be contained. We don't correctly allocate or + // generate code for a non-contained GT_FIELD_LIST. + noway_assert(!"Non-contained GT_FIELD_LIST"); + break; + + case GT_LIST: case GT_ARGPLACE: case GT_NO_OP: case GT_START_NONGC: diff --git a/src/jit/lsraarm64.cpp b/src/jit/lsraarm64.cpp index 1f367915d1..6497ac877a 100644 --- a/src/jit/lsraarm64.cpp +++ b/src/jit/lsraarm64.cpp @@ -87,8 +87,13 @@ void LinearScan::BuildNode(GenTree* tree) BuildStoreLoc(tree->AsLclVarCommon()); break; - case GT_LIST: case GT_FIELD_LIST: + // These should always be contained. We don't correctly allocate or + // generate code for a non-contained GT_FIELD_LIST. + noway_assert(!"Non-contained GT_FIELD_LIST"); + break; + + case GT_LIST: case GT_ARGPLACE: case GT_NO_OP: case GT_START_NONGC: diff --git a/src/jit/lsraxarch.cpp b/src/jit/lsraxarch.cpp index b0e95ae31f..5abdb4b6f3 100644 --- a/src/jit/lsraxarch.cpp +++ b/src/jit/lsraxarch.cpp @@ -111,8 +111,13 @@ void LinearScan::BuildNode(GenTree* tree) BuildStoreLoc(tree->AsLclVarCommon()); break; - case GT_LIST: case GT_FIELD_LIST: + // These should always be contained. We don't correctly allocate or + // generate code for a non-contained GT_FIELD_LIST. + noway_assert(!"Non-contained GT_FIELD_LIST"); + break; + + case GT_LIST: case GT_ARGPLACE: case GT_NO_OP: case GT_START_NONGC: diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.il b/tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.il new file mode 100644 index 0000000000..8ca630e1d7 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.il @@ -0,0 +1,151 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +// The bug that this test captures was a case where a call to a divide helper +// was being removed as dead code, but the GT_FIELD_LIST node for the long +// argument was left. Such nodes must always be contained, and if they become +// dead, all of their children must be marked as unused. + +.assembly extern mscorlib { auto } +.assembly extern System.Runtime { auto } +.assembly extern System.Console { auto } + +.assembly DevDiv_544983 { } + +.class public auto ansi beforefieldinit DevDiv_544983 + extends [System.Runtime]System.Object +{ + .method public hidebysig static int32 + Test(int32 i) cil managed noinlining + { + .locals init ([0] int32 dummy) + ldloc.0 + not + ldc.r8 -4.5508785095998289e-253 + conv.r8 + conv.ovf.u4.un + conv.ovf.u8.un + conv.r.un + ldarg 0x0 + conv.r.un + dup + cgt + conv.r.un + div + conv.u1 + conv.ovf.i8 + pop + ret + } // end of method DevDiv_544983:Test + + .method public hidebysig static int16 + Test2(uint16 a, float32 b, int32 c, uint16 d, uint8 e, int16 f, int32 g, int64 h) + { + .locals init ([0] int64 i) + + ldc.i8 2 + stloc.0 + ldarg.s 0x2 + ldarg.s 0x5 + div + conv.ovf.u2.un + conv.ovf.u2.un + ldarg 0x1 + conv.ovf.i4 + conv.ovf.i8.un + ldc.i4 0x669FC58A + neg + shl + conv.r.un + ldloc.s 0x0 + conv.ovf.i8.un + conv.r.un + ldloc 0x0 + conv.r.un + div + ceq + cgt.un + conv.r8 + conv.r.un + pop + ldloc 0x0 + neg + not + conv.i2 + ret + } + + .method public hidebysig static int32 Main() cil managed + { + .entrypoint + .locals init ([0] int32 retVal, [1] int32 testResult) + + ldc.i4.s 100 + stloc.0 + ldc.i4.1 + call int32 DevDiv_544983::Test(int32) + stloc.1 + ldloc.1 + ldc.i4.m1 + ceq + brtrue.s L2 + + ldstr "Test Result = " + call void [System.Console]System.Console::Write(string) + ldloc.1 + call void [System.Console]System.Console::WriteLine(int32) + ldstr "FAIL" + call void [System.Console]System.Console::WriteLine(string) + ldc.i4.m1 + stloc.0 + + L2: ldc.i4.1 + ldc.r4 2.0 + ldc.i4.3 + ldc.i4.4 + ldc.i4 5 + ldc.i4.6 + ldc.i4.7 + ldc.i8 8 + call int16 DevDiv_544983::Test2(uint16, float32, int32, uint16, uint8, int16, int32, int64) + stloc.1 + ldloc.1 + ldc.i4.1 + ceq + brtrue.s L3 + + ldstr "Test2 Result = " + call void [System.Console]System.Console::Write(string) + ldloc.1 + call void [System.Console]System.Console::WriteLine(int32) + ldstr "FAIL" + call void [System.Console]System.Console::WriteLine(string) + ldc.i4.m1 + stloc.0 + br L4 + + L3: ldloc.0 + ldc.i4.s 100 + ceq + brfalse.s L4 + + ldstr "PASS" + call void [System.Console]System.Console::WriteLine(string) + + L4: ldloc.0 + ret + } // end of method DevDiv_544983:Main + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 8 (0x8) + .maxstack 8 + ldarg.0 + call instance void [System.Runtime]System.Object::.ctor() + ret + } // end of method DevDiv_544983:.ctor + +} // end of class DevDiv_544983 + diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.ilproj b/tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.ilproj new file mode 100644 index 0000000000..5934cf63ac --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_544983/DevDiv_544983.ilproj @@ -0,0 +1,23 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + ..\..\ + + + + + None + True + + + + + + + -- cgit v1.2.3