From e7d1d4b8ee8640edf07e8732bf2f0c4e4fb092f9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 21:19:17 +0200 Subject: [PATCH 1/5] JIT: Evaluate GDV call args early The normal evaluation order for a callvirt is the following: 1. The 'this' arg is evaluated 2. The arguments are evaluated 3. 'this' is null-checked 4. The call is performed Step 1 and 2 happen as part of the IL instructions that load the arguments, while step 3 and 4 happen as part of the callvirt IL instruction. For GDV the guards needs to dereference 'this'. We were doing this too early, causing step 3 to happen before step 2. The fix is to spill all arguments for GDVs to temps. Fix #75607 --- src/coreclr/jit/indirectcalltransformer.cpp | 45 +++++++++++++-------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index e3d799f734b8ba..466636dca2964d 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -540,28 +540,39 @@ class IndirectCallTransformer checkBlock = currBlock; checkBlock->bbJumpKind = BBJ_COND; - CallArg* thisArg = origCall->gtArgs.GetThisArg(); - GenTree* thisTree = thisArg->GetNode(); - - // Create temp for this if the tree is costly. - if (thisTree->IsLocal()) - { - thisTree = compiler->gtCloneExpr(thisTree); - } - else + // The guard is going to dereference 'this'. This needs to happen + // after arguments have been evaluated if 'this' may be null, so we + // have to spill all arguments with side effects. + for (CallArg& arg : origCall->gtArgs.Args()) { - const unsigned thisTempNum = compiler->lvaGrabTemp(true DEBUGARG("guarded devirt this temp")); - GenTree* asgTree = compiler->gtNewTempAssign(thisTempNum, thisTree); - Statement* asgStmt = compiler->fgNewStmtFromTree(asgTree, stmt->GetDebugInfo()); - compiler->fgInsertStmtAtEnd(checkBlock, asgStmt); + bool spill = false; + GenTree* node = arg.GetNode(); - thisTree = compiler->gtNewLclvNode(thisTempNum, TYP_REF); + if (arg.GetWellKnownArg() == WellKnownArg::ThisPointer) + { + // Guard is going to access this arg, spill it if it is + // complex, regardless of side effects. + spill = !node->IsLocal(); + } + else + { + spill = (arg.GetNode()->gtFlags & GTF_SIDE_EFFECT) != 0; + } + + if (spill) + { + unsigned tmpNum = compiler->lvaGrabTemp(true DEBUGARG("guarded devirt arg temp")); + GenTree* asgTree = compiler->gtNewTempAssign(tmpNum, arg.GetNode()); + Statement* asgStmt = compiler->fgNewStmtFromTree(asgTree, stmt->GetDebugInfo()); + compiler->fgInsertStmtAtEnd(checkBlock, asgStmt); - // Propagate the new this to the call. Must be a new expr as the call - // will live on in the else block and thisTree is used below. - thisArg->SetEarlyNode(compiler->gtNewLclvNode(thisTempNum, TYP_REF)); + arg.SetEarlyNode(compiler->gtNewLclvNode(tmpNum, genActualType(arg.GetNode()))); + } } + CallArg* thisArg = origCall->gtArgs.GetThisArg(); + GenTree* thisTree = compiler->gtCloneExpr(thisArg->GetNode()); + // Remember the current last statement. If we're doing a chained GDV, we'll clone/copy // all the code in the check block up to and including this statement. // From 9b3789dc218a736afefa18fe2de32c1930a0c9fe Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 21:26:57 +0200 Subject: [PATCH 2/5] Add regression test --- .../JitBlue/Runtime_75607/Runtime_75607.cs | 63 +++++++++++++++++++ .../Runtime_75607/Runtime_75607.csproj | 24 +++++++ 2 files changed, 87 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs b/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs new file mode 100644 index 00000000000000..76e04eee9a7c56 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs @@ -0,0 +1,63 @@ +using System; +using System.Runtime.CompilerServices; +using System.Threading; + +public class Program +{ + private static int s_result; + public static int Main() + { + C c = new(); + for (int i = 0; i < 100; i++) + { + Foo(c); + Thread.Sleep(15); + } + + s_result = -1; + try + { + Foo(null); + Console.WriteLine("FAIL: No exception thrown"); + return -2; + } + catch (NullReferenceException) + { + if (s_result == 100) + { + Console.WriteLine("PASS"); + } + else + { + Console.WriteLine("FAIL: Result is {0}", s_result); + } + + return s_result; + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Foo(Base b) + { + b.Test(SideEffect(10)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static long SideEffect(long i) + { + s_result = 100; + return i; + } +} + +public interface Base +{ + public abstract void Test(long arg); +} + +public class C : Base +{ + public void Test(long arg) + { + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.csproj new file mode 100644 index 00000000000000..9a3423ff0d39b1 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.csproj @@ -0,0 +1,24 @@ + + + Exe + + + None + True + + + + + + + + + From 5c6efd7e73359c2f91a334266c420671f8d3dade Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 21:31:45 +0200 Subject: [PATCH 3/5] Test nit --- src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs b/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs index 76e04eee9a7c56..4d3a9999b6c987 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75607/Runtime_75607.cs @@ -52,7 +52,7 @@ private static long SideEffect(long i) public interface Base { - public abstract void Test(long arg); + void Test(long arg); } public class C : Base From 7c671e52d1b00b2ba3e0ea26a2a3492634cea87d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Sep 2022 21:34:39 +0200 Subject: [PATCH 4/5] CSE --- src/coreclr/jit/indirectcalltransformer.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 466636dca2964d..5813b5f4d18d87 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -545,28 +545,28 @@ class IndirectCallTransformer // have to spill all arguments with side effects. for (CallArg& arg : origCall->gtArgs.Args()) { - bool spill = false; - GenTree* node = arg.GetNode(); + bool spill = false; + GenTree* argNode = arg.GetNode(); if (arg.GetWellKnownArg() == WellKnownArg::ThisPointer) { // Guard is going to access this arg, spill it if it is // complex, regardless of side effects. - spill = !node->IsLocal(); + spill = !argNode->IsLocal(); } else { - spill = (arg.GetNode()->gtFlags & GTF_SIDE_EFFECT) != 0; + spill = (argNode->gtFlags & GTF_SIDE_EFFECT) != 0; } if (spill) { unsigned tmpNum = compiler->lvaGrabTemp(true DEBUGARG("guarded devirt arg temp")); - GenTree* asgTree = compiler->gtNewTempAssign(tmpNum, arg.GetNode()); + GenTree* asgTree = compiler->gtNewTempAssign(tmpNum, argNode); Statement* asgStmt = compiler->fgNewStmtFromTree(asgTree, stmt->GetDebugInfo()); compiler->fgInsertStmtAtEnd(checkBlock, asgStmt); - arg.SetEarlyNode(compiler->gtNewLclvNode(tmpNum, genActualType(arg.GetNode()))); + arg.SetEarlyNode(compiler->gtNewLclvNode(tmpNum, genActualType(argNode))); } } From 9d1f7de4f4c8663bc0552dd93a5af5f0ef0a1e15 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 15 Sep 2022 12:11:58 +0200 Subject: [PATCH 5/5] Spill non-side-effecting previous arguments properly Arguments with global references or order side effects may need to happen before a later argument with side effects is evaluated. Since we do not have accurate GTF_GLOB_REF at this point use gtHasLocalsWithAddrOp which does a full tree walk checking for lvHasLdAddrOp. --- src/coreclr/jit/indirectcalltransformer.cpp | 63 ++++++++++++++------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 5813b5f4d18d87..6af4872473855d 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -540,37 +540,42 @@ class IndirectCallTransformer checkBlock = currBlock; checkBlock->bbJumpKind = BBJ_COND; - // The guard is going to dereference 'this'. This needs to happen - // after arguments have been evaluated if 'this' may be null, so we - // have to spill all arguments with side effects. + // Find last arg with a side effect. All args with any effect + // before that will need to be spilled. + CallArg* lastSideEffArg = nullptr; for (CallArg& arg : origCall->gtArgs.Args()) { - bool spill = false; - GenTree* argNode = arg.GetNode(); - - if (arg.GetWellKnownArg() == WellKnownArg::ThisPointer) - { - // Guard is going to access this arg, spill it if it is - // complex, regardless of side effects. - spill = !argNode->IsLocal(); - } - else + if ((arg.GetNode()->gtFlags & GTF_SIDE_EFFECT) != 0) { - spill = (argNode->gtFlags & GTF_SIDE_EFFECT) != 0; + lastSideEffArg = &arg; } + } - if (spill) + if (lastSideEffArg != nullptr) + { + for (CallArg& arg : origCall->gtArgs.Args()) { - unsigned tmpNum = compiler->lvaGrabTemp(true DEBUGARG("guarded devirt arg temp")); - GenTree* asgTree = compiler->gtNewTempAssign(tmpNum, argNode); - Statement* asgStmt = compiler->fgNewStmtFromTree(asgTree, stmt->GetDebugInfo()); - compiler->fgInsertStmtAtEnd(checkBlock, asgStmt); + GenTree* argNode = arg.GetNode(); + if (((argNode->gtFlags & GTF_ALL_EFFECT) != 0) || compiler->gtHasLocalsWithAddrOp(argNode)) + { + SpillArgToTempBeforeGuard(&arg); + } - arg.SetEarlyNode(compiler->gtNewLclvNode(tmpNum, genActualType(argNode))); + if (&arg == lastSideEffArg) + { + break; + } } } - CallArg* thisArg = origCall->gtArgs.GetThisArg(); + CallArg* thisArg = origCall->gtArgs.GetThisArg(); + // We spill 'this' if it is complex, regardless of side effects. It + // is going to be used multiple times due to the guard. + if (!thisArg->GetNode()->IsLocal()) + { + SpillArgToTempBeforeGuard(thisArg); + } + GenTree* thisTree = compiler->gtCloneExpr(thisArg->GetNode()); // Remember the current last statement. If we're doing a chained GDV, we'll clone/copy @@ -674,6 +679,22 @@ class IndirectCallTransformer compiler->fgInsertStmtAtEnd(checkBlock, jmpStmt); } + //------------------------------------------------------------------------ + // SpillArgToTempBeforeGuard: spill an argument into a temp in the guard/check block. + // + // Parameters + // arg - The arg to create a temp and assignment for. + // + void SpillArgToTempBeforeGuard(CallArg* arg) + { + unsigned tmpNum = compiler->lvaGrabTemp(true DEBUGARG("guarded devirt arg temp")); + GenTree* asgTree = compiler->gtNewTempAssign(tmpNum, arg->GetNode()); + Statement* asgStmt = compiler->fgNewStmtFromTree(asgTree, stmt->GetDebugInfo()); + compiler->fgInsertStmtAtEnd(checkBlock, asgStmt); + + arg->SetEarlyNode(compiler->gtNewLclvNode(tmpNum, genActualType(arg->GetNode()))); + } + //------------------------------------------------------------------------ // FixupRetExpr: set up to repair return value placeholder from call //