From a08818ead21c46fa18d2390438ae5b3425165bc0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 27 Apr 2023 19:41:50 +0200 Subject: [PATCH] JIT: Handle GT_RETFILT in fgInsertStmtNearEnd (#85420) Also add some logging for read backs in physical promotion. Fix #85088 --- src/coreclr/jit/fgstmt.cpp | 22 ++++++---- src/coreclr/jit/promotion.cpp | 8 ++-- .../JitBlue/Runtime_85088/Runtime_85088.cs | 50 ++++++++++++++++++++++ .../JitBlue/Runtime_85088/Runtime_85088.csproj | 9 ++++ 4 files changed, 77 insertions(+), 12 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj diff --git a/src/coreclr/jit/fgstmt.cpp b/src/coreclr/jit/fgstmt.cpp index 9094061..7fa2250 100644 --- a/src/coreclr/jit/fgstmt.cpp +++ b/src/coreclr/jit/fgstmt.cpp @@ -171,7 +171,7 @@ Statement* Compiler::fgNewStmtAtEnd(BasicBlock* block, GenTree* tree, const Debu //------------------------------------------------------------------------ // fgInsertStmtNearEnd: Insert the given statement at the end of the given basic block, -// but before the GT_JTRUE, if present. +// but before the terminating node, if present. // // Arguments: // block - the block into which 'stmt' will be inserted; @@ -182,7 +182,7 @@ void Compiler::fgInsertStmtNearEnd(BasicBlock* block, Statement* stmt) // This routine can only be used when in tree order. assert(fgOrder == FGOrderTree); - if (block->KindIs(BBJ_COND, BBJ_SWITCH, BBJ_RETURN)) + if (block->KindIs(BBJ_EHFINALLYRET, BBJ_EHFAULTRET, BBJ_EHFILTERRET, BBJ_COND, BBJ_SWITCH, BBJ_RETURN)) { Statement* firstStmt = block->firstStmt(); noway_assert(firstStmt != nullptr); @@ -191,24 +191,28 @@ void Compiler::fgInsertStmtNearEnd(BasicBlock* block, Statement* stmt) Statement* insertionPoint = lastStmt->GetPrevStmt(); #if DEBUG - if (block->bbJumpKind == BBJ_COND) + if (block->KindIs(BBJ_COND)) { - assert(lastStmt->GetRootNode()->gtOper == GT_JTRUE); + assert(lastStmt->GetRootNode()->OperIs(GT_JTRUE)); } - else if (block->bbJumpKind == BBJ_RETURN) + else if (block->KindIs(BBJ_RETURN)) { - assert((lastStmt->GetRootNode()->gtOper == GT_RETURN) || (lastStmt->GetRootNode()->gtOper == GT_JMP) || + assert((lastStmt->GetRootNode()->OperIs(GT_RETURN, GT_JMP)) || // BBJ_RETURN blocks in functions returning void do not get a GT_RETURN node if they // have a .tail prefix (even if canTailCall returns false for these calls) // code:Compiler::impImportBlockCode (search for the RET: label) // Ditto for real tail calls (all code after them has been removed) - ((lastStmt->GetRootNode()->gtOper == GT_CALL) && + (lastStmt->GetRootNode()->OperIs(GT_CALL) && ((info.compRetType == TYP_VOID) || lastStmt->GetRootNode()->AsCall()->IsTailCall()))); } + else if (block->KindIs(BBJ_EHFINALLYRET, BBJ_EHFAULTRET, BBJ_EHFILTERRET)) + { + assert(lastStmt->GetRootNode()->OperIs(GT_RETFILT)); + } else { - assert(block->bbJumpKind == BBJ_SWITCH); - assert(lastStmt->GetRootNode()->gtOper == GT_SWITCH); + assert(block->KindIs(BBJ_SWITCH)); + assert(lastStmt->GetRootNode()->OperIs(GT_SWITCH)); } #endif // DEBUG diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 7315cef..9b8cafb 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1770,11 +1770,13 @@ PhaseStatus Promotion::Run() assert(!rep.NeedsReadBack || !rep.NeedsWriteBack); if (rep.NeedsReadBack) { - JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u at the end of " FMT_BB "\n", i, + JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u near the end of " FMT_BB ":\n", i, rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, bb->bbNum); - GenTree* readBack = replacer.CreateReadBack(i, rep); - m_compiler->fgInsertStmtNearEnd(bb, m_compiler->fgNewStmtFromTree(readBack)); + GenTree* readBack = replacer.CreateReadBack(i, rep); + Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); + DISPSTMT(stmt); + m_compiler->fgInsertStmtNearEnd(bb, stmt); rep.NeedsReadBack = false; } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.cs b/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.cs new file mode 100644 index 0000000..f6ff6fd --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.cs @@ -0,0 +1,50 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_85088 +{ + [Fact] + public static int Test() + { + Foo f = new(); + try + { + try + { + throw new Exception(); + } + finally + { + f.X = 15; + f.Y = 20; + f.X += f.Y; + f.Y *= f.X; + + // f will be physically promoted and will require a read back after this call. + // Since this is a finally, some platforms will have a GT_RETFILT that we were + // inserting IR after instead of before. + f = Call(f); + } + } + catch + { + } + + return f.X + f.Y; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static Foo Call(Foo f) + { + return new Foo { X = 75, Y = 25 }; + } + + private struct Foo + { + public short X, Y; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj new file mode 100644 index 0000000..85f04c1 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj @@ -0,0 +1,9 @@ + + + True + + + + + + \ No newline at end of file -- 2.7.4