Skip to content

Conversation

@AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented May 15, 2025

Morph currently applies assertions in preorder, so updates made by morph or by local assertion prop to child trees cannot be leveraged when considering parents.

For instance if we had

V01 = newarr
V05 = V01
if (V05 == null)

morph was unable to resolve the branch, despite generating both V01 != null and V05 == V01.

To address this, keep track of the assertions valid at the end of each statement (via apLocalPostorder), and use those assertions during morph postorder to try and optimize further. This assertion set is kept up to date with any subsequent kills, and (for simplicity) cleared out if there is a qmark.

Note morph can't use the current (apLocal) assertion set in postorder because morph may also reorder subtrees, so in postorder might end up using assertions generated by (formerly) later-executing subtrees to optimize now earlier-executing subtrees.

@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 15, 2025
@dotnet-policy-service
Copy link
Contributor

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

@AndyAyersMS AndyAyersMS changed the title proof of concept JIT: also run local assertion prop in postorder during morph May 16, 2025
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented May 16, 2025

Diffs

Nice CQ improvement and TP improvement.

Big CQ diffs (both good and bad) seem to be loop cloning related, at least from my limited sample.

@dotnet/jit-contrib PTAL

@AndyAyersMS AndyAyersMS marked this pull request as ready for review May 16, 2025 04:44
Copilot AI review requested due to automatic review settings May 16, 2025 04:44
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 PR enables postorder local assertion propagation during morphing to leverage assertions generated in child subtrees when optimizing parent nodes.

  • Tracks a new bitvector apLocalPostorder alongside the existing apLocal.
  • Integrates a postorder assertion-prop loop in fgMorphSmpOp guarded by a new config flag.
  • Updates assertion-kill logic and block/statement initialization to maintain the postorder set.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/coreclr/jit/morph.cpp Added apLocalPostorder clear on qmark, integrated postorder assertion-prop loop and diff on kills
src/coreclr/jit/jitconfigvalues.h Introduced JitEnablePostorderLocalAssertionProp config option
src/coreclr/jit/compiler.h Declared new apLocalPostorder bitvector in Compiler class
Comments suppressed due to low confidence (1)

src/coreclr/jit/morph.cpp:7757

  • Consider adding a JIT unit test that enables JitEnablePostorderLocalAssertionProp and verifies that a pattern like newarr alias + null check is resolved postorder, to prevent regressions.
if (fgGlobalMorph && optLocalAssertionProp && (optAssertionCount > 0))

Metrics.LocalAssertionOverflow = optAssertionOverflow;
Metrics.MorphTrackedLocals = lvaTrackedCount;
Metrics.MorphLocals = lvaCount;
optLocalAssertionProp = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

curious - is this something you noticed or it was prevention from getting into weird state, now that we will do another round of assertions with apLocalPostorder?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make sure calls into morph after the global morph phase didn't try and use local assertions. Most uses of local assertions in morph are guarded by fgGlobalMorph but it is easy to forget...

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Diffs looks great. Wondering have we investigated why there are some regressions and if there is any pattern out there?

@AndyAyersMS
Copy link
Member Author

Diffs looks great. Wondering have we investigated why there are some regressions and if there is any pattern out there?

The big regressions seem to be extra loop cloning. I only checked a handful but suspect that's generally the case.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr pgostress, runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Optional pipelines have some known failures, so some result screening will be needed.

@AndyAyersMS
Copy link
Member Author

Stress failures all look like known issues or timeouts.

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