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.

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

unblock cloning of loops where the header is a try begin #108604

Conversation

AndyAyersMS
Copy link
Member

No description provided.

@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 7, 2024
Copy link
Contributor

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

@AndyAyersMS
Copy link
Member Author

@jakobbotsch semi-hacky fix for #96887. Frequently unblocks cloning of foreach over IEnumerable. EG #62457 (comment) now becomes:

Considering loop L00 to clone for optimizations.
Analyzing iteration for L00 with header BB12
  Preheader = BB11
  Checking exiting block BB20
    Could not extract an IV
  Checking exiting block BB18
    Could not extract an IV
  Checking exiting block BB14
    Could not extract an IV
  Could not find any IV
Checking loop L00 for optimization candidates (GDV tests)
...GDV considering [000067]
... right form for type test with local V02
Loop L00 has invariant type test [000067] on V02
Checking whether cloning is profitable ...
  Yes
...GDV considering [000012]
...GDV considering [000195]
...GDV considering [000052]
... right form for type test with local V02
Loop L00 has invariant type test [000052] on V02
Checking whether cloning is profitable ...
  Yes

and this gives some small perf wins as we can avoid the Enumerator GDVs within the "hot" loop.

Method Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
f3q (base) 780.87 ns 2.398 ns 2.243 ns 3.42 0.02 0.0048 32 B 0.02
f3q (diff) 679.55 ns 1.553 ns 1.377 ns 2.89 0.02 0.0048 32 B 0.02

"Ratio" is comparing to a for loop, so we're still paying a lot for not being able to promote the enumerator.

@AndyAyersMS
Copy link
Member Author

Diffs

Some very large size increases in the PGO collections, from GDV-driven cloning. Seems like we may need to put some rudimentary profitability heuristics on cloning to keep this under control.

Some size improvements from layout and some new loop hoisting / cse.

Jakob has also suggested doing the complementary thing when the loop crosses a try -- putting the try inside the loop (with header and backedge sources outside) instead of putting the loop inside the try.

@AndyAyersMS
Copy link
Member Author

Merging to pick up #108771

@AndyAyersMS
Copy link
Member Author

Updated Diffs.

Code size and TP both increase but should be viewed in conjunction with the even bigger decreases seen in #108771.

@AndyAyersMS AndyAyersMS marked this pull request as ready for review October 15, 2024 14:51
@AndyAyersMS
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

{
if (backedgeSource->hasTryIndex())
{
preheaderEHRegion = backedgeSource->getTryIndex();
Copy link
Member

Choose a reason for hiding this comment

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

Previously we were using ehTrueEnclosingTryIndexIL here, but not anymore. Was it unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it was arguably better.

If there is a backedge source in a non-contained EH region we should use the true enclosing region of the header to vet any existing preheader, rather than a region based on the backedge source.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. No diffs (locally, x64) from the previous version.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM with a question

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants