From: Carol Eidt Date: Tue, 7 Aug 2018 15:58:59 +0000 (-0700) Subject: Fix evaluation order for block copy X-Git-Tag: submit/tizen/20210909.063632~11030^2~4095^2~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=e67cb1df302bf2a1f530cdbbd16d671064c2f66c;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fix evaluation order for block copy This order was changed five years ago, as a workaround for data flow analysis issues around the block copy. Now that block copies are handled as regular assignments, this is no longer needed - and is, in fact, incorrect as it causes side effects to be handled in the wrong order. However, for the case where the lhs is an indirection of a local address, it must be evaluated 2nd for SSA renaming to be correct. In the process, also remove the special-casing for the size operand of `DYN_BLK` under `ASG`. Fix dotnet/coreclr#19243 Commit migrated from https://github.com/dotnet/coreclr/commit/16e5914706e8865a35da769abe7e6f5da4c910eb --- diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index d98cf2f..cb7c476 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -18494,36 +18494,24 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR) } // Special handling for dynamic block ops. - if (tree->OperIsDynBlkOp()) + if (tree->OperIs(GT_DYN_BLK, GT_STORE_DYN_BLK)) { - GenTreeDynBlk* dynBlk; - GenTree* src; - GenTree* asg = tree; - if (tree->OperGet() == GT_ASG) - { - dynBlk = tree->gtGetOp1()->AsDynBlk(); - src = tree->gtGetOp2(); - } - else - { - dynBlk = tree->AsDynBlk(); - src = dynBlk->Data(); - asg = nullptr; - } - GenTree* sizeNode = dynBlk->gtDynamicSize; - GenTree* dstAddr = dynBlk->Addr(); + GenTreeDynBlk* dynBlk = tree->AsDynBlk(); + GenTree* sizeNode = dynBlk->gtDynamicSize; + GenTree* dstAddr = dynBlk->Addr(); + GenTree* src = dynBlk->Data(); + bool isReverse = ((dynBlk->gtFlags & GTF_REVERSE_OPS) != 0); if (dynBlk->gtEvalSizeFirst) { fgSetTreeSeqHelper(sizeNode, isLIR); } - if (tree->gtFlags & GTF_REVERSE_OPS) + if (isReverse && (src != nullptr)) { fgSetTreeSeqHelper(src, isLIR); - fgSetTreeSeqHelper(dstAddr, isLIR); } - else + fgSetTreeSeqHelper(dstAddr, isLIR); + if (!isReverse && (src != nullptr)) { - fgSetTreeSeqHelper(dstAddr, isLIR); fgSetTreeSeqHelper(src, isLIR); } if (!dynBlk->gtEvalSizeFirst) @@ -18531,10 +18519,6 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR) fgSetTreeSeqHelper(sizeNode, isLIR); } fgSetTreeSeqFinish(dynBlk, isLIR); - if (asg != nullptr) - { - fgSetTreeSeqFinish(asg, isLIR); - } return; } diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index af59dfe..f9844d6 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -4203,17 +4203,33 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) switch (op1Val->gtOper) { case GT_IND: + case GT_BLK: + case GT_OBJ: + case GT_DYN_BLK: - // Struct assignments are different from scalar assignments in that semantically - // the address of op1 is evaluated prior to op2. - if (!varTypeIsStruct(op1)) + // In an indirection, the destination address is evaluated prior to the source. + // If we have any side effects on the target indirection, + // we have to evaluate op1 first. + // However, if the LHS is a lclVar address, SSA relies on using evaluation order for its + // renaming, and therefore the RHS must be evaluated first. + // If we have an assignment involving a lclVar address, the LHS may be marked as having + // side-effects. + // However the side-effects won't require that we evaluate the LHS address first: + // - The GTF_GLOB_REF might have been conservatively set on a FIELD of a local. + // - The local might be address-exposed, but that side-effect happens at the actual assignment (not + // when its address is "evaluated") so it doesn't change the side effect to "evaluate" the address + // after the RHS (note that in this case it won't be renamed by SSA anyway, but the reordering is + // safe). + // + if (op1Val->AsIndir()->Addr()->IsLocalAddrExpr()) { - // If we have any side effects on the GT_IND child node - // we have to evaluate op1 first. - if (op1Val->gtOp.gtOp1->gtFlags & GTF_ALL_EFFECT) - { - break; - } + bReverseInAssignment = true; + tree->gtFlags |= GTF_REVERSE_OPS; + break; + } + if (op1Val->AsIndir()->Addr()->gtFlags & GTF_ALL_EFFECT) + { + break; } // In case op2 assigns to a local var that is used in op1Val, we have to evaluate op1Val first. @@ -4233,9 +4249,6 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_LCL_VAR: case GT_LCL_FLD: - case GT_BLK: - case GT_OBJ: - case GT_DYN_BLK: // We evaluate op2 before op1 bReverseInAssignment = true; @@ -6912,9 +6925,6 @@ void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVa result->gtFlags |= dst->gtFlags & GTF_ALL_EFFECT; result->gtFlags |= result->gtOp.gtOp2->gtFlags & GTF_ALL_EFFECT; - // REVERSE_OPS is necessary because the use must occur before the def - result->gtFlags |= GTF_REVERSE_OPS; - result->gtFlags |= (dst->gtFlags & GTF_EXCEPT) | (srcOrFillVal->gtFlags & GTF_EXCEPT); if (isVolatile) @@ -8411,15 +8421,6 @@ unsigned GenTree::NumChildren() } } #endif - // Special case for assignment of dynamic block. - // This is here to duplicate the former case where the size may be evaluated prior to the - // source and destination addresses. In order to do this, we treat the size as a child of the - // assignment. - // TODO-1stClassStructs-Cleanup: Remove all this special casing, and ensure that the diffs are reasonable. - if ((OperGet() == GT_ASG) && (gtOp.gtOp1->OperGet() == GT_DYN_BLK) && (gtOp.gtOp1->AsDynBlk()->gtEvalSizeFirst)) - { - return 3; - } assert(gtOp.gtOp1 != nullptr); if (gtOp.gtOp2 == nullptr) { @@ -8454,17 +8455,8 @@ unsigned GenTree::NumChildren() case GT_ARR_ELEM: return 1 + AsArrElem()->gtArrRank; - // This really has two children, but if the size is evaluated first, we treat it as a child of the - // parent assignment. case GT_DYN_BLK: - if (AsDynBlk()->gtEvalSizeFirst) - { - return 1; - } - else - { - return 2; - } + return 2; case GT_ARR_OFFSET: case GT_STORE_DYN_BLK: @@ -8604,10 +8596,9 @@ GenTree* GenTree::GetChild(unsigned childNum) switch (childNum) { case 0: - return AsDynBlk()->Addr(); + return AsDynBlk()->gtEvalSizeFirst ? AsDynBlk()->gtDynamicSize : AsDynBlk()->Addr(); case 1: - assert(!AsDynBlk()->gtEvalSizeFirst); - return AsDynBlk()->gtDynamicSize; + return AsDynBlk()->gtEvalSizeFirst ? AsDynBlk()->Addr() : AsDynBlk()->gtDynamicSize; default: unreached(); } diff --git a/src/coreclr/src/jit/rationalize.cpp b/src/coreclr/src/jit/rationalize.cpp index 89e4167..8a4115c 100644 --- a/src/coreclr/src/jit/rationalize.cpp +++ b/src/coreclr/src/jit/rationalize.cpp @@ -551,7 +551,10 @@ void Rationalizer::RewriteAssignment(LIR::Use& use) (assignment->gtFlags & (GTF_ALL_EFFECT | GTF_BLK_VOLATILE | GTF_BLK_UNALIGNED | GTF_DONT_CSE)); storeBlk->gtBlk.Data() = value; - // Replace the assignment node with the store + // Remove the block node from its current position and replace the assignment node with it + // (now in its store form). + BlockRange().Remove(storeBlk); + BlockRange().InsertBefore(assignment, storeBlk); use.ReplaceWith(comp, storeBlk); BlockRange().Remove(assignment); DISPTREERANGE(BlockRange(), use.Def()); diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243.cs new file mode 100644 index 0000000..68cd912 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243.cs @@ -0,0 +1,46 @@ +// 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. +// +// This test exposed a bug with the ordering of evaluation of a cpblk. + +struct S0 +{ + public long F0; + public sbyte F4; + public S0(long f0): this() { F0 = f0; } +} + +class C0 +{ + public S0 F5; + public C0(S0 f5) { F5 = f5; } +} + +public class GitHub_19243 +{ + static C0 s_13 = new C0(new S0(0)); + static S0 s_37; + + public static int checkValue(long value) + { + if (value != 8614979244451975600L) + { + System.Console.WriteLine("s_37.F0 was " + value + "; expected 8614979244451975600L"); + return -1; + } + return 100; + } + + static ref S0 M7() + { + s_13 = new C0(new S0(8614979244451975600L)); + return ref s_37; + } + + public static int Main() + { + M7() = s_13.F5; + return checkValue(s_37.F0); + } +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_r.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_r.csproj new file mode 100644 index 0000000..82611b7 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_r.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + 1 + + + + + + + False + + + + None + False + + + + + + + + + + diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_ro.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_ro.csproj new file mode 100644 index 0000000..15d9d64 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19243/GitHub_19243_ro.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + 1 + + + + + + + False + + + + None + True + + + + + + + + + +