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

Merge return true/false blocks #94387

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Merge return true/false blocks #94387

merged 1 commit into from
Nov 9, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 5, 2023 •
edited
Loading

I plan to unconditionally expand all return <condition> blocks to JTRUE, merge tails (this PR) and then fold to return <condition>; back if tails were not found.

Diffs

@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 Nov 5, 2023
@ghost ghost assigned EgorBo Nov 5, 2023
@ghost
Copy link

ghost commented Nov 5, 2023

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

Issue Details

CI experiment, some nice diffs locally

Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr
Milestone: -

@EgorBo EgorBo marked this pull request as ready for review November 5, 2023 21:56
This was referenced Nov 5, 2023
Closed
Closed
@EgorBo EgorBo marked this pull request as draft November 6, 2023 13:24
@build-analysis build-analysis bot mentioned this pull request Nov 8, 2023
Open
@build-analysis build-analysis bot mentioned this pull request Nov 8, 2023
Closed
3 tasks
@EgorBo
Copy link
Member Author

EgorBo commented Nov 8, 2023 •
edited
Loading

@AndyAyersMS @jakobbotsch @dotnet/jit-contrib PTAL, the current shape is what @jakobbotsch suggested in #94387 (comment) (basically, his branch), my initial one was focusing on return <bool> and turned out to be too conservative.

I've limitted it to single-statement RETURN blocks, but it seems like it makes sense to enable it for all, but that will require some careful analysis of TP/Size regressions (up to +0.4% TP impact) + needs a small fix in morph for explicit tail calls (I or Jakob will try to land it separately).
There 3 source of regressions:

  1. Loop clonning (esp in aspnet collection), example is https://www.diffchecker.com/doR6p4zH/ (checked via JitDump that it's indeed loop clonning)
  2. In some cases we no longer emit branchless cmove, etc due to a different layout of blocks
  3. In some cases we light up a frame in x64 leaf functions, my understanding that it's because of that BB count heuristic

but overall it's a clear size win: https://dev.azure.com/dnceng-public/public/_build/results?buildId=463538&view=results

I also expect to see some improvements when I expand return <compare> into jtrue separately

@EgorBo EgorBo marked this pull request as ready for review November 8, 2023 20:31
jakobbotsch
jakobbotsch reviewed Nov 8, 2023
View reviewed changes
src/coreclr/jit/block.cpp Outdated
}

const Statement* stmt = firstStmt();
while (ignoreNopStatements && ((stmt->GetRootNode() == nullptr) || (stmt->GetRootNode()->OperIs(GT_NOP))))
Copy link
Member

Choose a reason for hiding this comment

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

Is a statement with a null node possible? Overall, do we actually find any NOPs in the return blocks?

Copy link
Member Author

@EgorBo EgorBo Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't sure in this, but if I remove the nullcheck it won't fail on CI so I presume it's not possible? I wish we could have a centralized "IR reference" where we could check what's legal and what's not in IR

Copy link
Member Author

Choose a reason for hiding this comment

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

Is a statement with a null node possible? Overall, do we actually find any NOPs in the return blocks?

I'll check the diffs

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted these changes, there were no additional diffs from that

jakobbotsch
jakobbotsch reviewed Nov 8, 2023
View reviewed changes
src/coreclr/jit/fgopt.cpp
predInfo.Emplace(predBlock, lastStmt);
}

auto tailMergePreds = [&](BasicBlock* commSucc) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to convert these to functions instead of massive lambdas... but that's a no-diff refactoring to be done separately.

jakobbotsch
jakobbotsch reviewed Nov 8, 2023
View reviewed changes
src/coreclr/jit/gentree.cpp Outdated
@@ -2519,6 +2519,12 @@ bool GenTreeCall::Equals(GenTreeCall* c1, GenTreeCall* c2)
return false;
}

// Make sure "explicit tail call" flags match.
if (c1->IsTailPrefixedCall() != c2->IsTailPrefixedCall())
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be reverted (and fixed in the future PR), or we should check all the tailcall flags.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

@AndyAyersMS
Copy link
Member

I've limitted it to single-statement RETURN blocks, but it seems like it makes sense to enable it for all, but that will require some careful analysis of TP/Size regressions (up to +0.4% TP impact) + needs a small fix in morph for explicit tail calls (I or Jakob will try to land it separately).

I'm surprised checkin more statements has such a TP impact; it should be fairly self-limiting as there can't be that many return blocks. Unless it's enabling a lot more merging?

AndyAyersMS
AndyAyersMS reviewed Nov 8, 2023
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.

I don't have anything to add other that what's already been noted.

Cool that this was relatively refactoring simple; I recall working on this a while back but got bogged down for some reason.

@EgorBo
Copy link
Member Author

EgorBo commented Nov 8, 2023

I'm surprised checkin more statements has such a TP impact;

it's possible that the TP regressions in that case come from extra loop clonning etc, I'll check separately, this PR will help to isolate those changes

@EgorBo EgorBo merged commit e4d4736 into dotnet:main Nov 9, 2023
127 of 129 checks passed
@EgorBo EgorBo deleted the dedup-bbjret-cond branch November 9, 2023 12:27
@AndyAyersMS AndyAyersMS mentioned this pull request Nov 14, 2023
Open
This was referenced Nov 14, 2023
Open
Open
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS left review comments

@SingleAccretion SingleAccretion SingleAccretion left review comments

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@EgorBo EgorBo

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.
4 participants
@EgorBo @AndyAyersMS @jakobbotsch @SingleAccretion