JIT: improve codegen for inline candidates called only for effect by AndyAyersMS · Pull Request #116123 · dotnet/runtime

AndyAyersMS · 2025-05-30T00:57:32Z

Keep track of unused GT_RET_EXPRs, and when we inline, substitute just the return value side effects for unused GT_RET_EXPRs.

Reduces address exposure in some GDV cases, which can encourage physical promotion and other good things.

In particular one case from #84872.

Keep track of unused GT_RET_EXPRs, and when we inline, substitute just the return value side effects for unused GT_RET_EXPRs. Reduces address exposure in some GDV cases, which can encourage physical promotion and other good things. In particular one case from dotnet#84872.

AndyAyersMS · 2025-05-30T00:58:00Z

@EgorBo PTAL
cc @dotnet/jit-contrib

Modest diffs; will link to them when they're up.

dotnet-policy-service · 2025-05-30T00:58:21Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull Request Overview

This PR enhances JIT inlining to handle GT_RET_EXPR nodes whose return values are unused, by tracking them and substituting side effects only when appropriate.

Introduces a new GTF_RET_EXPR_UNUSED flag and helper methods (IsUnused, SetUnused) for GenTreeRetExpr.
Updates IndirectCallTransformer and the inline substitution walker to convert unused or void GT_RET_EXPRs into NOPs or extract side effects.
Refines IR validation and side‐effect analysis to skip over GT_COMMA(NOP,NOP) patterns and nested GT_RET_EXPR chains.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/coreclr/jit/morph.cpp	Adds `IsCommaNop` helper and special-cases comma-NOP nodes in tail-call IR validation.
src/coreclr/jit/indirectcalltransformer.cpp	Refactors inline-candidate handling to patch unused/void returns.
src/coreclr/jit/gentree.h	Defines `GTF_RET_EXPR_UNUSED` and methods on `GenTreeRetExpr`.
src/coreclr/jit/gentree.cpp	Prints unused markers and updates side-effect detection loop.
src/coreclr/jit/fginline.cpp	Enhances placeholder substitution to extract side effects for unused returns.
src/coreclr/jit/compiler.hpp	Marks `GT_RET_EXPR` as unused in `gtUnusedValNode`.

Comments suppressed due to low confidence (3)

src/coreclr/jit/morph.cpp:5110

The helper IsCommaNop is defined below as a nested function, but C++ does not allow nested function definitions. Move IsCommaNop out of the method body and declare it as a private member or free function.

else if (IsCommaNop(tree))

src/coreclr/jit/morph.cpp:5123

This definition appears inside the implementation file rather than at file or class scope, leading to invalid C++ syntax. Refactor it into an appropriate scope (e.g. a private static method of Compiler).

bool IsCommaNop(GenTree* node)

src/coreclr/jit/gentree.cpp:16886

This while loop never updates potentialCall, leading to an infinite loop whenever the initial node is GT_RET_EXPR. Consider changing it back to if or updating potentialCall within the loop.

while (potentialCall->OperIs(GT_RET_EXPR))

src/coreclr/jit/gentree.h

EgorBo · 2025-05-30T12:18:50Z

src/coreclr/jit/compiler.hpp

+    {
+        JITDUMP("\nmarking RET_EXPR [%06u] as unused\n", dspTreeID(expr));
+        expr->AsRetExpr()->SetUnused();
+    }


To be honest, it is a bit weird to mutate input/state in gtNew-like APIs (but I think we do that in a few others).

EgorBo · 2025-05-30T12:21:27Z

src/coreclr/jit/morph.cpp

+                return false;
+            }
+
+            return node->AsOp()->gtGetOp1()->OperIs(GT_NOP) && node->AsOp()->gtGetOp2()->OperIs(GT_NOP);


Can it be a recursive COMMA(nop, COMMA(nop, nop)) ? perhaps we should use some gtEffectiveVal-like api here?

Good question. I haven't seen this.

EgorBo · 2025-05-30T12:23:27Z

src/coreclr/jit/gentree.cpp

+            GenTreeRetExpr* const retExpr    = tree->AsRetExpr();
+            GenTreeCall* const    inlineCand = retExpr->gtInlineCandidate;
+
+            if ((retExpr->gtFlags & GTF_RET_EXPR_UNUSED) != 0)


haven't you just added IsUnused() API ?

EgorBo

LGTM with a few nits

jakobbotsch · 2025-05-30T12:38:27Z

src/coreclr/jit/fginline.cpp

+            // If this use is an unused ret expr, is the first child of a comma, the return value is ignored.
+            // Extract any side effects.
+            //
+            if (isUnused || ((parent != nullptr) && parent->OperIs(GT_COMMA) && (parent->gtGetOp1() == *use)))


Why is the parent check not sufficient? Adding a contextual flag on RET_EXPR seems weird and fragile.

Point taken, but note RET_EXPRs are already weird and fragile (can't be cloned, etc).

The main motivation was to give the new RET_EXPR created during GDV expansion the same benefit as the original one; in that setting we don't know the RET_EXPR parent.

FWIW this extra handing in GDV expansion is responsible for about 20% of the diffs in ASP.NET.

I suppose this early in the phase list it's likely the ret expr will be in the next statement. So GDV expansion can search there for its parent; if it fails it can just assume the ret expr is used. I'll give that a try.

Got it, I think I understand the GDV case better now. Another +1 for inlining via tree splitting I guess...

AndyAyersMS · 2025-05-30T18:16:36Z

Revised this to search for the ret expr during GDV instead of marking it up in the importer.

Same behavior on the test case and asp.net.

AndyAyersMS · 2025-05-30T20:28:26Z

@EgorBo I've revised; take another look?

EgorBo

Copilot AI review requested due to automatic review settings May 30, 2025 00:57

github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 30, 2025

dotnet-policy-service bot assigned AndyAyersMS May 30, 2025

AndyAyersMS mentioned this pull request May 30, 2025

Fold GDV checks for inlined delegates #84872

Closed

Copilot AI reviewed May 30, 2025

View reviewed changes

AndyAyersMS requested a review from Copilot May 30, 2025 01:11

Copilot AI reviewed May 30, 2025

View reviewed changes

src/coreclr/jit/gentree.h Outdated Show resolved Hide resolved

This was referenced May 30, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Test failure: baseservices/threading/regressions/115178/115178/115178.cmd #116060

Closed

EgorBo reviewed May 30, 2025

View reviewed changes

EgorBo approved these changes May 30, 2025

View reviewed changes

jakobbotsch reviewed May 30, 2025

View reviewed changes

don't annotate; search for the ret expr parent instead

a925e60

EgorBo approved these changes May 30, 2025

View reviewed changes

AndyAyersMS merged commit 2411805 into dotnet:main May 30, 2025
109 checks passed

AndyAyersMS mentioned this pull request Jun 17, 2025

JIT: De-abstraction in .NET 10 #108913

Closed

github-actions bot locked and limited conversation to collaborators Jun 30, 2025

Conversation

AndyAyersMS commented May 30, 2025

Uh oh!

AndyAyersMS commented May 30, 2025

Uh oh!

dotnet-policy-service bot commented May 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

EgorBo May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo May 30, 2025

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS May 30, 2025

Choose a reason for hiding this comment

Uh oh!

EgorBo May 30, 2025

Choose a reason for hiding this comment

Uh oh!

EgorBo left a comment

Choose a reason for hiding this comment

Uh oh!

jakobbotsch May 30, 2025

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS May 30, 2025

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS May 30, 2025

Choose a reason for hiding this comment

Uh oh!

jakobbotsch May 30, 2025

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented May 30, 2025

Uh oh!

AndyAyersMS commented May 30, 2025

Uh oh!

EgorBo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EgorBo May 30, 2025 •

edited

Loading