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

JIT: enable edge checks throughout #99628

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

AndyAyersMS
Copy link
Member

Fix the last remaining issues for edge likelihoods.

Main challenge here was switch lowering, particularly the expansions of switches into a series of tests. The adjustments here are similar to those for multi-guess GDV and type tests -- as we test possibilities one-by-one we have to adjust and scale up likelihoods of remining possibilities. But for switches things are more complex as edges may have dup counts, and we may eventually reach the point where the remaining tests had zero initial likelihood.

Contributes to #93020.

Fix the last remaining issues for edge likelihoods.

Main challenge here was switch lowering, particularly the expansions of switches
into a series of tests. The adjustments here are similar to those for multi-guess
GDV and type tests -- as we test possibilities one-by-one we have to adjust and
scale up likelihoods of remining possibilities. But for switches things are more
complex as edges may have dup counts, and we may eventually reach the point where
the remaining tests had zero initial likelihood.

Contributes to dotnet#93020.
@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 Mar 12, 2024
@AndyAyersMS
Copy link
Member Author

@amanasifkhalid PTAL
cc @dotnet/jit-contrib

At long last, edge has likelihood and likelihood sum checks are enabled throughout. Some diffs expected as the new block weights for switch expansions impact LSRA ordering.

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

LGTM, just a few small nits. Also, I've run into JitStress issues with some of the switch lowering paths during previous flowgraph refactors, so it might be a good idea to run some of the outerloop pipelines on this.

src/coreclr/jit/lower.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/lower.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/lower.cpp Show resolved Hide resolved
@AndyAyersMS
Copy link
Member Author

LGTM, just a few small nits. Also, I've run into JitStress issues with some of the switch lowering paths during previous flowgraph refactors, so it might be a good idea to run some of the outerloop pipelines on this.

Looks like I don't even need to go to outerloop... issues on arm to sort out.

@AndyAyersMS
Copy link
Member Author

LGTM, just a few small nits. Also, I've run into JitStress issues with some of the switch lowering paths during previous flowgraph refactors, so it might be a good idea to run some of the outerloop pipelines on this.

Looks like I don't even need to go to outerloop... issues on arm to sort out.

There is a complication with dup edges, not sure how to fix just yet. We have a switch

BB02  [0001]  1       BB01                  0      0 [003..018)-> BB04,BB05,BB04,BB03[def] (switch)

So cases 0 and 2 are dup'd to BB04.

We split BB02 into BB02->def, and new BB115 for the switch.

We expand the switch as series of tests; case 0 decides it can reuse BB115. We add ref BB115->BB04, this does not create a new edge but shares the existing one; later this messes up likelihood because we assume that the BB115->BB04 edge is new and not shared.

@AndyAyersMS
Copy link
Member Author

Likely we should modify setLikelhood to tolerate slight overflow and underflow, rather than forcing each computation to do so. Will do this in a follow-up PR.

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes -- thanks!

@AndyAyersMS
Copy link
Member Author

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

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member Author

One libraries jitstress failure to sort out.

@AndyAyersMS
Copy link
Member Author

#99740 should have fixed the libraries stress issue. Going to merge up and retest.

@AndyAyersMS
Copy link
Member Author

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

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Libraries jitstress failures look like #99725.

Diffs

@AndyAyersMS
Copy link
Member Author

Runtime failures are known to build analysis (not sure why the didn't get linked here).

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.

None yet

2 participants