Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JIT: Clean up inliner substitution #77176

Merged
merged 6 commits into from
Oct 21, 2022
Merged

JIT: Clean up inliner substitution #77176

merged 6 commits into from
Oct 21, 2022

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Oct 18, 2022 •
edited
Loading

  • Stop using GenTree::ReplaceWith
  • GenTreeRetExpr now directly contains its substitution members, which is a tree either from the inline candidate, a local (due to spill temps) or the call itself (due to inliner failure). In the former case, it also contains the BasicBlock the child tree comes from to ensure that its flags can be propagated once and for all during substitution.
  • Make various members strongly typed, e.g. GenTreeRetExpr::gtInlineCandidate is now a GenTreeCall*, InlineCandidateInfo::retExpr is now a GenTreeRetExpr*. This is possible since we are no longer using GenTree::ReplaceWith.

Fix #75827

@jakobbotsch
JIT: Clean up inliner substitution
5f164ff 
* Stop using GenTree::ReplaceWith
* GenTreeRetExpr now directly contains its substitution members, which
  is a tree either from the inline candidate, a local (due to spill
  temps) or the call itself (due to inliner failure). In the former
  case, it also contains the BasicBlock the child tree comes from to
  ensure that its flags can be propagated once and for all during
  substitution.
* Make various members strongly typed, e.g.
  GenTreeRetExpr::gtInlineCandidate is now a GenTreeCall*,
  InlineCandidateInfo::retExpr is now a GenTreeRetExpr*. This is
  possible since we are no longer using GenTree::ReplaceWith.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 18, 2022
@ghost ghost assigned jakobbotsch Oct 18, 2022
@ghost
Copy link

ghost commented Oct 18, 2022

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

Issue Details
  • Stop using GenTree::ReplaceWith
  • GenTreeRetExpr now directly contains its substitution members, which is a tree either from the inline candidate, a local (due to spill temps) or the call itself (due to inliner failure). In the former case, it also contains the BasicBlock the child tree comes from to ensure that its flags can be propagated once and for all during substitution.
  • Make various members strongly typed, e.g. GenTreeRetExpr::gtInlineCandidate is now a GenTreeCall*, InlineCandidateInfo::retExpr is now a GenTreeRetExpr*. This is possible since we are no longer using GenTree::ReplaceWith.
Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Oct 19, 2022
Closed
@jakobbotsch
Propagate BB flags properly on inline failures
37086f7 
When we fail to inline, the substitution will be the call itself, so we
should propagate flags from the basic block containing the call. In
particular this can be a problem with RET_EXPR chains.
@jakobbotsch
Fix dumping, retain odd previous behavior, fix GDV substitution
802ac66
@jakobbotsch
More comments
0177368
@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member

Getting the block flags right has been a persistent source of trouble; see eg #37335, #37840, #38243 and probably a few more.

@jakobbotsch
Copy link
Member Author

Getting the block flags right has been a persistent source of trouble; see eg #37335, #37840, #38243 and probably a few more.

Indeed, and I'm pretty sure the current way we propagate does not really make logical sense (see TODOs I have added in the source in this PR). I am hoping we are going to make some inliner changes in this release that we can remove those TODOs as part of.

At least this PR accomplishes centralizing all the substitution logic (well, except for GDV) into UpdateInlineReturnExpressionPlaceHolder. Hopefully this is the end of the wrong flag propagation, but I suppose we'll see...

@jakobbotsch
Copy link
Member Author

The problem seems simple enough -- whenever we replace a GT_RET_EXPR node with an arbitrary tree, we potentially need to propagate flags from the basic block that the arbitrary tree comes from. But the previous behavior using GenTree::ReplaceWith makes it very difficult to reason about the arbitrary trees and where they are coming from once you start replacing GT_RET_EXPR nodes and the inline candidates with other GT_RET_EXPR nodes. This PR was initially just a clean up to try to simplify the convoluted replacement logic, but turned out to fix the issue too.

@build-analysis build-analysis bot mentioned this pull request Oct 19, 2022
Closed
@jakobbotsch jakobbotsch marked this pull request as ready for review October 19, 2022 19:52
@jakobbotsch
Copy link
Member Author

jakobbotsch commented Oct 19, 2022 •
edited
Loading

cc @dotnet/jit-contrib PTAL @AndyAyersMS @EgorBo

Small number of diffs where nested inlining propagates flags in slightly different ways.

@build-analysis build-analysis bot mentioned this pull request Oct 19, 2022
Closed
2 tasks
AndyAyersMS
AndyAyersMS approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider fixing the "prev BB" and BBF_SPLIT_GAINED issues as separate isolated PRs (both from this PR and from each other by a few days), so we can better assess the impact of these via the lab data.

@jakobbotsch
Copy link
Member Author

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

The failures look like #76880 and known from build analysis.

Consider fixing the "prev BB" and BBF_SPLIT_GAINED issues as separate isolated PRs (both from this PR and from each other by a few days), so we can better assess the impact of these via the lab data.

Makes sense.

@jakobbotsch
Copy link
Member Author

I cannot reproduce the Fuzzlyn failures on the head of my PR. That probably means they are recent regressions in main.

@jakobbotsch
Copy link
Member Author

I'm not able to reproduce it on main either. I have seen these kind of intermittent failures before, so I don't think it's related to this PR. Not sure how we can track these down, I will need to think about if I can record more information from Fuzzlyn.

The failures looked known as mentioned above so will merge.

@jakobbotsch jakobbotsch merged commit c34ccb1 into dotnet:main Oct 21, 2022
@jakobbotsch jakobbotsch deleted the fix-75827 branch October 21, 2022 17:53
@ghost ghost locked as resolved and limited conversation to collaborators Nov 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

@EgorBo EgorBo Awaiting requested review from EgorBo

Assignees

@jakobbotsch jakobbotsch

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[libraries-pgo] BBF_HAS_IDX_LEN is not set ... but is required because of the following tree
2 participants
@jakobbotsch @AndyAyersMS