[experiment] Move importervectorization to a late phase#129068
Closed
EgorBo wants to merge 4 commits into
Closed
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
Member
Author
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reworks CoreCLR JIT handling of half-constant string / span comparisons by removing the importer-time vectorization path and shifting constant folding / unrolling to a later VN-based folding path. It also introduces a new named intrinsic for System.Globalization.Ordinal.EqualsIgnoreCase and adjusts CoreLib inlining attributes to expose patterns that the late-phase optimizer can recognize.
Changes:
- Remove importer-phase vectorization (
importervectorization.cpp) and associated intrinsic handling inimpIntrinsic. - Add a new named intrinsic (
NI_System_String_OrdinalEqualsIgnoreCase) and enable VN-based folding/unrolling for it alongsideSpanHelpers.SequenceEqual. - Add
gtFoldExprConstChainand use it to recover “almost constant” lengths so VN-based folding can still trigger after assertion propagation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs | Adds AggressiveInlining to several [Intrinsic] string comparison overloads to increase pattern visibility for later optimization. |
| src/libraries/System.Private.CoreLib/src/System/Globalization/Ordinal.cs | Marks Ordinal.EqualsIgnoreCase as [Intrinsic] and NoInlining to keep a recognizable call for late folding/unrolling. |
| src/coreclr/jit/namedintrinsiclist.h | Adds NI_System_String_OrdinalEqualsIgnoreCase enum value. |
| src/coreclr/jit/importervectorization.cpp | Removes importer-time string/span unroll+vectorization implementation. |
| src/coreclr/jit/importercalls.cpp | Removes importer-time vectorization cases; adds intrinsic identification for System.Globalization.Ordinal.EqualsIgnoreCase and marks it “special”. |
| src/coreclr/jit/gentree.cpp | Adds gtFoldExprConstChain helper (recursive fold into constants). |
| src/coreclr/jit/fgprofile.cpp | Ensures the new intrinsic is still instrumented under minimal profiling. |
| src/coreclr/jit/compiler.h | Declares gtFoldExprConstChain; removes importer-vectorization helper declarations; updates optVNBasedFoldExpr_Call signature. |
| src/coreclr/jit/CMakeLists.txt | Drops importervectorization.cpp from JIT sources. |
| src/coreclr/jit/assertionprop.cpp | Expands VN-based folding for SequenceEqual and the new intrinsic, including constant-fold and one-sided-constant unrolling. |
Member
Author
GetImmutableDataFromAddress could read the content of frozen objects and of static readonly fields' own bytes, but not the data referenced through a movable static readonly reference field (e.g. a 'static readonly string' allocated on the normal heap). The length of such fields already folds via the ARR_LENGTH path using getStaticFieldContent(ignoreMovableObjects: false); this mirrors that for content so optVNBasedFoldExpr_Call_Memcmp can bake the characters. The new path recognizes [cns +] InvariantNonNullLoad(fieldSeq) of TYP_REF, reads the (possibly movable) object handle with ignoreMovableObjects=false, and bakes its immutable content via getObjectContent. Guarded by TYP_REF (so primitive fields aren't misread as handles) and isObjectImmutable (so mutable objects aren't baked). The movable handle itself is never embedded into codegen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
…eDataFromAddress The static-readonly-reference-field path called ValueNumStore::PeelOffsets unconditionally on the address VN. For some callers (e.g. optVNBasedFoldExpr_Call_Memmove) the address node's liberal VN can be NoVN (TYP_UNDEF) or otherwise not TYP_BYREF/TYP_I_IMPL/TYP_REF, which trips the PeelOffsets type assert and fails fast (seen broadly in CI). Only attempt the peel + InvariantNonNullLoad recognition when the address VN is actually an address type. This preserves the string-folding (byref addresses pass the guard) while avoiding the assert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ld/offset - optVNBasedFoldExpr_Call_Memcmp: gtCloneExpr can (in principle) fail; treat a null clone as 'length not constant' instead of dereferencing it. - GetImmutableDataFromAddress: require fseq->IsStaticField() before calling getStaticFieldContent, and bound the peeled offset by INT32_MAX before passing it to getObjectContent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
2 tasks
3 tasks
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.
Should also catch more patterns