Move CHECK_IR before morph global#127352
Closed
AndyAyersMS wants to merge 1 commit intodotnet:mainfrom
Closed
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT behavior in two areas: (1) it enables CHECK_IR phase checks before PHASE_MORPH_GLOBAL so post-phase IR validation runs after global morph, and (2) it adds logic to re-derive Vector<T> sizing/ISA markers from the runtime-reported Vector<T> struct size (via getClassSize) during intrinsic lookup and struct normalization.
Changes:
- Enable
PhaseChecks::CHECK_IRprior to global morph sofgDebugCheckLinks()runs on morphed IR. - Infer
Vector<T>width (128/256/512) fromVector<T>’s class size and adjust VectorT ISA markers accordingly. - Add instruction set flag normalization (
Set64BitInstructionSetVariants+EnsureInstructionSetFlagsAreValid) incompSetProcessor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/coreclr/jit/importercalls.cpp | Adjusts Vector<T> intrinsic lookup sizing based on class size and updates VectorT ISA markers. |
| src/coreclr/jit/importer.cpp | Updates struct normalization to detect System.Numerics.Vector<T> and adjust VectorT ISA markers based on its size. |
| src/coreclr/jit/compiler.cpp | Tweaks ISA initialization/validation and moves CHECK_IR enabling ahead of global morph. |
Comment on lines
+10844
to
+10873
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | ||
|
|
||
| switch (classSize) | ||
| { | ||
| case 16: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | ||
| size = 16; | ||
| break; | ||
|
|
||
| case 32: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | ||
| size = 32; | ||
| break; | ||
|
|
||
| case 64: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); |
There was a problem hiding this comment.
Avoid mutating opts.compSupportsISAReported / opts.compSupportsISAExactly here. Those sets are intended to be managed by compExactlyDependsOn (which calls notifyInstructionSetUsage and then updates *Reported/Exactly); pre-populating them here can suppress VM notification for VectorT dependencies and make R2R/ISA tracking incorrect. Consider only adjusting opts.compSupportsISA (if absolutely necessary) and let compExactlyDependsOn report usage, or compute size from classSize without changing global ISA tracking state.
Suggested change
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | |
| switch (classSize) | |
| { | |
| case 16: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | |
| size = 16; | |
| break; | |
| case 32: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | |
| size = 32; | |
| break; | |
| case 64: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); | |
| switch (classSize) | |
| { | |
| case 16: | |
| size = 16; | |
| break; | |
| case 32: | |
| size = 32; | |
| break; | |
| case 64: |
Comment on lines
+10854
to
+10860
| switch (classSize) | ||
| { | ||
| case 16: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | ||
| size = 16; |
There was a problem hiding this comment.
This switch has no default/assert, but the code has already cleared all VectorT* flags above. If classSize is ever not 16/32/64 (e.g., unexpected layout), the compiler will silently proceed with no VectorT ISA set, leaving compilation state inconsistent. Consider guarding the flag update so it only happens for the expected sizes, and otherwise keep the existing VectorT* flags (or assert/unreached).
Comment on lines
+1202
to
+1226
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | ||
|
|
||
| switch (originalSize) | ||
| { | ||
| case 16: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | ||
| break; | ||
|
|
||
| case 32: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | ||
| break; | ||
|
|
||
| case 64: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); |
There was a problem hiding this comment.
Avoid mutating opts.compSupportsISAReported / opts.compSupportsISAExactly in impNormStructType. These sets are used to track which ISAs were actually reported to the VM (see compExactlyDependsOn); manually adding VectorT* here can prevent notifyInstructionSetUsage from running later and can corrupt dependency tracking. If you need Vector sizing to follow the struct size, prefer deriving a local size value (or adjusting only compSupportsISA during option setup) and leave *Reported/*Exactly to compExactlyDependsOn.
Suggested change
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | |
| switch (originalSize) | |
| { | |
| case 16: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | |
| break; | |
| case 32: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | |
| break; | |
| case 64: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); | |
| switch (originalSize) | |
| { | |
| case 16: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | |
| break; | |
| case 32: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | |
| break; | |
| case 64: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); |
Comment on lines
+1199
to
+1227
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | ||
|
|
||
| switch (originalSize) | ||
| { | ||
| case 16: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | ||
| break; | ||
|
|
||
| case 32: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | ||
| break; | ||
|
|
||
| case 64: | ||
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | ||
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); | ||
| break; |
There was a problem hiding this comment.
This switch clears all VectorT* flags first, but does nothing for unexpected originalSize values. If originalSize is not 16/32/64, the method will silently leave VectorT sizing unset for the rest of the compilation. Consider validating originalSize and either preserving the existing VectorT* flags or asserting to avoid silently changing global ISA state.
Suggested change
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | |
| switch (originalSize) | |
| { | |
| case 16: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | |
| break; | |
| case 32: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | |
| break; | |
| case 64: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); | |
| break; | |
| const bool isValidVectorTSize = (originalSize == 16) || (originalSize == 32) || (originalSize == 64); | |
| assert(isValidVectorTSize); | |
| if (isValidVectorTSize) | |
| { | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISA.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.RemoveInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.RemoveInstructionSet(InstructionSet_VectorT512); | |
| switch (originalSize) | |
| { | |
| case 16: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT128); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT128); | |
| break; | |
| case 32: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT256); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT256); | |
| break; | |
| case 64: | |
| opts.compSupportsISA.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAReported.AddInstructionSet(InstructionSet_VectorT512); | |
| opts.compSupportsISAExactly.AddInstructionSet(InstructionSet_VectorT512); | |
| break; | |
| } |
Member
Author
|
Just wanted to share this with Tanner |
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not sold on the need for these size updates, or (if needed) the way they're implemented