Skip to content

Conversation

@amanasifkhalid
Copy link
Contributor

@amanasifkhalid amanasifkhalid commented May 27, 2025

Succeeds #113709 (GitHub won't let me reopen it). Part of #107749 and #108901. Fixes #50204.

@Copilot Copilot AI review requested due to automatic review settings May 27, 2025 15:51
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

@amanasifkhalid amanasifkhalid requested a review from Copilot May 27, 2025 15:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements graph-based loop inversion in the JIT, aiming to improve IR transformation behavior and diagnostics.

  • In fgopt.cpp, the logic now returns true to indicate IR modifications in edge cases, and error handling for stmt cloning has been shifted to an assertion.
  • In fgdiagnostic.cpp, a more descriptive debug message is added for loop exits with non-loop predecessors.
  • In compiler.h, the API is updated to use a more specific function (optTryInvertWhileLoop) with a refined parameter type.

Reviewed Changes

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

File Description
src/coreclr/jit/fgopt.cpp Updated control flow for IR modification detection and error handling
src/coreclr/jit/fgdiagnostic.cpp Enhanced loop diagnostic messaging
src/coreclr/jit/compiler.h Updated API for loop inversion with adjusted parameter type
Comments suppressed due to low confidence (2)

src/coreclr/jit/fgopt.cpp:2713

  • Changing the response for non-RelOp conditions from returning false to returning true alters the behavior of IR modifications. Verify that this change is intentional and that downstream code correctly handles the modified IR state.
if (!condTree->OperIsCompare())

src/coreclr/jit/compiler.h:7123

  • The API change from 'optInvertWhileLoop(BasicBlock* block)' to 'optTryInvertWhileLoop(FlowGraphNaturalLoop* loop)' requires ensuring that all call sites and semantic expectations are updated accordingly.
bool optTryInvertWhileLoop(FlowGraphNaturalLoop* loop);

@amanasifkhalid
Copy link
Contributor Author

Diffs are now a net size reduction. Unsurprisingly, we have lots of size increases from finding new inversion candidates, as well as size decreases from bailing on large loops previously inverted by the lexical implementation. Whether we are doing more or less inversion overall depends on the collection. For example:

  • In aspnet, the number of loops found drops modestly from 41768 to 41481, and the number of loops inverted increases from 15939 to 16281.
  • In benchmarks.run_pgo, the number of loops found drops from 91864 to 86981, and the number of loops inverted decreases from 36156 to 31067.

@AndyAyersMS @jakobbotsch do these diffs seem like too much of a swing in the opposite direction?

@AndyAyersMS
Copy link
Member

@AndyAyersMS @jakobbotsch do these diffs seem like too much of a swing in the opposite direction?

I think the TP cost is ok here. For CQ I don't know what experiments to try other than running a lot of benchmarks, and for that the lab data will be a better guide. P6 snap looks like 6/13 so we have time to get data back and change our minds.

So let's try this and see.

@amanasifkhalid
Copy link
Contributor Author

@AndyAyersMS I think that's a reasonable plan, though we'll want #115850 merged in around the same time to see improvements in the list enumeration scenarios we're targeting. That change will incur a similarly large number of diffs; are you ok trying these back-to-back before the next snap?

@AndyAyersMS
Copy link
Member

@AndyAyersMS I think that's a reasonable plan, though we'll want #115850 merged in around the same time to see improvements in the list enumeration scenarios we're targeting. That change will incur a similarly large number of diffs; are you ok trying these back-to-back before the next snap?

Maybe not immeidately back to back -- a day apart?

@amanasifkhalid
Copy link
Contributor Author

a day apart?

Sure, sounds reasonable.

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.

Loop inversion should consider top-tested loops

3 participants