From c5f25f38e625076916ce48a5aa4d485b5a514c0f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 1 May 2022 12:54:11 +0200 Subject: [PATCH 1/3] Remove unnecessary GTF_GLOB_REF on locals GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually uses a tree with this flag as a value (and not as a location). This removes the flag in a few more places by using IsLocalAddrExpr. Fixes #68669 --- src/coreclr/jit/fgdiagnostic.cpp | 4 ++-- src/coreclr/jit/morph.cpp | 17 ++++++++++------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 1ddeaecffdfc7a..cf6e828882e7d1 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3085,8 +3085,8 @@ void Compiler::fgDebugCheckFlags(GenTree* tree) return GenTree::VisitResult::Continue; }); - // ADDR nodes break the "parent flags >= operands flags" invariant for GTF_GLOB_REF. - if (tree->OperIs(GT_ADDR) && op1->OperIs(GT_LCL_VAR, GT_LCL_FLD)) + // Addresses of locals never need GTF_GLOB_REF + if (tree->IsLocalAddrExpr()) { expectedFlags &= ~GTF_GLOB_REF; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 65b50de28f1108..36ee2eec7686ee 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11169,10 +11169,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) } } - /* Morphing along with folding and inlining may have changed the - * side effect flags, so we have to reset them - * - * NOTE: Don't reset the exception flags on nodes that may throw */ + // Morphing along with folding and inlining may have changed the + // side effect flags, so we have to reset them + // + // NOTE: Don't reset the exception flags on nodes that may throw assert(tree->gtOper != GT_CALL); @@ -11181,11 +11181,14 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) tree->gtFlags &= ~GTF_CALL; } - /* Propagate the new flags */ + // Propagate the new flags tree->gtFlags |= (op1->gtFlags & GTF_ALL_EFFECT); - // &aliasedVar doesn't need GTF_GLOB_REF, though alisasedVar does - if (oper == GT_ADDR && (op1->gtOper == GT_LCL_VAR)) + // addresses of locals do not need GTF_GLOB_REF, even if the child has + // it (is address exposed). Note that general addressing may still need + // GTF_GLOB_REF, for example if the subtree has a comma that involves a + // global reference. + if (((tree->gtFlags & GTF_GLOB_REF) != 0) && tree->IsLocalAddrExpr()) { tree->gtFlags &= ~GTF_GLOB_REF; } From 840ff24d7ab4380f047936a55abc600cbd8274dc Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 1 May 2022 13:00:49 +0200 Subject: [PATCH 2/3] Improve assertion for ret-buf as local optimization --- src/coreclr/jit/morph.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 36ee2eec7686ee..522a01280d29d9 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1769,6 +1769,12 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call) if (setupArg != nullptr) { arg.SetEarlyNode(setupArg); + + // Make sure we do not break recognition of retbuf-as-local + // optimization here. If this is hit it indicates that we are + // unnecessarily creating temps for some ret buf addresses, and + // gtCallGetDefinedRetBufLclAddr relies on this not to happen. + noway_assert((arg.GetWellKnownArg() != WellKnownArg::RetBuffer) || !call->IsOptimizingRetBufAsLocal()); } arg.SetLateNode(defArg); @@ -1776,10 +1782,6 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call) lateTail = &arg.LateNextRef(); } - // Make sure we did not do anything here that stops us from being able to - // find the local ret buf if we are optimizing it. - noway_assert(!call->IsOptimizingRetBufAsLocal() || (comp->gtCallGetDefinedRetBufLclAddr(call) != nullptr)); - #ifdef DEBUG if (comp->verbose) { From 0a744381f4c618c78e1ee21f1d3ac0dcd7484cd6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 1 May 2022 16:09:55 +0200 Subject: [PATCH 3/3] Readd GT_ADDR check --- src/coreclr/jit/fgdiagnostic.cpp | 2 +- src/coreclr/jit/morph.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index cf6e828882e7d1..d5b3cb54683e1e 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3086,7 +3086,7 @@ void Compiler::fgDebugCheckFlags(GenTree* tree) }); // Addresses of locals never need GTF_GLOB_REF - if (tree->IsLocalAddrExpr()) + if (tree->OperIs(GT_ADDR) && tree->IsLocalAddrExpr()) { expectedFlags &= ~GTF_GLOB_REF; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 522a01280d29d9..08c0308dba589e 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11190,7 +11190,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // it (is address exposed). Note that general addressing may still need // GTF_GLOB_REF, for example if the subtree has a comma that involves a // global reference. - if (((tree->gtFlags & GTF_GLOB_REF) != 0) && tree->IsLocalAddrExpr()) + if (tree->OperIs(GT_ADDR) && ((tree->gtFlags & GTF_GLOB_REF) != 0) && tree->IsLocalAddrExpr()) { tree->gtFlags &= ~GTF_GLOB_REF; }