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: Stop calling morph from CSE #106695

Merged
merged 6 commits into from
Oct 15, 2024

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Aug 20, 2024

CSE cannot tolerate morph removing any defs, which many of morphs transformations can do. We have been slowly infecting morph with more and more checks to avoid this, but in reality CSE just should not reinvoke morph -- the benefits of doing this are minimal.

Fix #108600

CSE cannot tolerate morph removing any defs, which many of morphs
transformations can do. We have been slowly infecting morph with more
and more checks to avoid this, but in reality CSE just should not
reinvoke morph -- the benefits of doing this are minimal.
@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr superpmi-diffs

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet-policy-service dotnet-policy-service bot removed this from the 10.0.0 milestone Oct 3, 2024
@jakobbotsch jakobbotsch reopened this Oct 7, 2024
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo

Diffs. Regressions in cases where some morph optimizations no longer kick in after CSE, but I think they're small enough that we can accept them and see if something shows up in micro benchmarks before porting more transformations to lowering. Some TP improvements now that we avoid the morph call.

@EgorBo
Copy link
Member

EgorBo commented Oct 14, 2024

I am curious why we didn't do it before instead of fixing morph with these checks 🙂 I presume some parts of morph also conservatively use fgGlobalMorph check

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM, agree that diffs aren't too bad to fix a popular source of bugs

@jakobbotsch
Copy link
Member Author

I am curious why we didn't do it before instead of fixing morph with these checks 🙂

Good question, I guess it was simpler to just keep adding those checks.

I presume some parts of morph also conservatively use fgGlobalMorph check

Ah yeah, some of those can probably be cleaned up. We still call morph from some other places, like assertion prop, but it's probably not as sensitive as CSE. I'll open an issue about investigating those.

I also realized that I missed removing checks for optValnumCSE_phase from morph. Let me push a commit that removes those too...

@jakobbotsch jakobbotsch merged commit 07f15ef into dotnet:main Oct 15, 2024
104 of 107 checks passed
@jakobbotsch jakobbotsch deleted the stop-morphing-from-cse branch October 15, 2024 08:15
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2024
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.

JIT: Assertion failed 'link' during 'Optimize Valnum CSEs'
2 participants