JIT: Materialize partially-constant Vector.Create as CNS_VEC + inserts - #130717
JIT: Materialize partially-constant Vector.Create as CNS_VEC + inserts#130717tannergooding wants to merge 2 commits into
Conversation
IsHWIntrinsicCreateConstant now optionally reports which elements are constant via a simdmask_t. When two or more operands of a Vector.Create are non-zero constants, lowering materializes them as a single vector constant and inserts the remaining non-constant operands via WithElement, rather than emitting a chain of inserts starting from CreateScalarUnsafe. Only non-zero constants are counted towards this threshold: zero lanes are already produced for free by the existing insert path (insertps zero-masking, CreateScalarUnsafe zero-extension), so materializing them into a constant would add instructions rather than remove them. Larger vectors are still split into per-128-bit-lane Create nodes before reaching the base case, so the optimization applies per lane and a fully constant lane collapses to a CNS_VEC combined via WithUpper/WithLower. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR improves JIT lowering for partially-constant Vector*.Create(...) by materializing multiple non-zero constant lanes as a single vector constant (CNS_VEC) and inserting only the remaining non-constant lanes, reducing node count and typically improving codegen.
Changes:
- Extend
GenTreeVecCon::IsHWIntrinsicCreateConstantto optionally return a per-element constant mask (simdmask_t). - Add a shared lowering path (
LowerHWIntrinsicCreateWithInserts) that builds aCNS_VECand fills non-constant operands viaWithElement/platform insert intrinsics. - Add a profitability helper (
NonZeroConstantElementCount) and wire it into xarch/armarchLowerHWIntrinsicCreatefor 128-bit lanes.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/lowerxarch.cpp | Uses the new constant-mask + non-zero-constant threshold to prefer CNS_VEC + WithElement over insert chains for partially-constant creates. |
| src/coreclr/jit/lowerarmarch.cpp | Same as xarch: enables the new partial-constant materialization path on ARM64 lowering. |
| src/coreclr/jit/lower.h | Declares the new helper lowering method and profitability helper. |
| src/coreclr/jit/lower.cpp | Implements NonZeroConstantElementCount and LowerHWIntrinsicCreateWithInserts shared lowering logic. |
| src/coreclr/jit/gentree.h | Extends IsHWIntrinsicCreateConstant to optionally report which lanes were constant via simdmask_t. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 5
The helper checks the raw bytes so that -0.0 (sign bit set) is counted as non-zero, since the free zeroing paths would otherwise turn it into +0.0. Reword the comments to say all-bits-zero rather than non-zero so the intent is clear and not simplified to a numeric == 0 check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@MihuBot -nuget |
|
CC. @dotnet/jit-contrib, @EgorBo. This one should be ready for review. Not as many hits as I would've liked, likely because the pattern isn't overfly common, but a few in ImageSharp and the numerics types. Also improves Base64Encode under PGO Namely this just recognizes the Code is centralized and shared across Arm64 and Xarch to avoid duplication, so we can easily hook it up for WASM next as well. |
I explicitly avoided it in the past in favour of const + insert in the past cuz it wasn't handled. |
|
Right, its what I've recommended and I imagine most would've done if they profiled due to lack of handling So code can be cleaned up now if this goes in. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "87e0c9c6ed68d23f0126b0d16cb7672a90b2460a",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "dab6af23aabd9ec6ca1eb5a02822c449eb1e013f",
"last_reviewed_commit": "87e0c9c6ed68d23f0126b0d16cb7672a90b2460a",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "dab6af23aabd9ec6ca1eb5a02822c449eb1e013f",
"last_recorded_worker_run_id": "29684014079",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "87e0c9c6ed68d23f0126b0d16cb7672a90b2460a",
"review_id": 4730634801
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: LowerHWIntrinsicCreate materialized a partially-constant Vector.Create (e.g. Vector128.Create(1, 2, 3, x)) as a chain of inserts starting from CreateScalarUnsafe, emitting an insert even for the constant lanes. That is more nodes and more instructions than necessary when several lanes are known constants.
Approach: IsHWIntrinsicCreateConstant now optionally reports a per-element constant mask via simdmask_t. A new profitability helper NonZeroConstantElementCount counts constant lanes whose raw bytes are not all-zero, and when at least two qualify, the shared LowerHWIntrinsicCreateWithInserts materializes a single CNS_VEC from the constant lanes and inserts only the remaining non-constant operands via WithElement. The threshold deliberately ignores all-bits-zero lanes because the existing insert path zeroes them for free (insertps zero-masking, CreateScalarUnsafe zero-extension), so counting them could regress. The helper checks raw bytes rather than numeric value so -0.0 (sign bit set) is correctly treated as non-zero, since the free zeroing paths would otherwise produce +0.0. Both the xarch and armarch LowerHWIntrinsicCreate gate on the same helper at the per-128-bit-lane base case.
Summary: The change is well-scoped, correct, and consistent with the surrounding lowering conventions. The new insert loop mirrors the existing pattern (LIR::LastNode insertion point to minimize lifetimes, LowerNode on each insert, TryGetUse/SetUnusedValue result handling, and the !TARGET_64BIT OperIsLong operand cleanup). The constant-mask plumbing preserves the original IsHWIntrinsicCreateConstant return contract, and the new cnsMask out-parameter defaults to nullptr so existing callers are unaffected. The profitability reasoning around zero vs. non-zero lanes and -0.0 is subtle but correct and is captured in thorough header comments. The reported SuperPMI asmdiffs (-806 bytes, no regressions) and the described validation across base types, vector sizes, ISA configs, and JitStress=2 align with the reasoning in the code. I found no correctness or performance concerns. LGTM.
Detailed Findings
No actionable findings. The memcpy into vecCon->gtSimdVal uses simdSize, which is bounded by the 16-byte lane invariant asserted upstream; the simdmask_t single-u64 storage is sufficient because this path only runs on per-128-bit lanes (at most 16 elements); and WithElement lowering already covers all base types reached here, so replacing the type-specific insert chain is safe.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 91.6 AIC · ⌖ 10.5 AIC · ⊞ 10K
LowerHWIntrinsicCreatepreviously materialized a partially-constantVector.Createas a chain of inserts starting fromCreateScalarUnsafe, even for the elements that were constant. For example,Vector128.Create(1, 2, 3, x)produced four inserts rather than a single constant plus one insert forx.IsHWIntrinsicCreateConstantnow optionally reports which elements are constant via asimdmask_t. When two or more operands are non-zero constants, lowering materializes them as a singleCNS_VECand inserts only the remaining non-constant operands viaWithElement. SoVector128.Create(1, 2, 3, x)becomes aCNS_VECplus a singleWithElement.Only non-zero constants are counted towards the threshold. Zero lanes are already produced for free by the existing insert path (
insertpszero-masking,CreateScalarUnsafezero-extension), so materializing them into a constant would add instructions rather than remove them -- for exampleVector4.Create(a, b, 0, 0)stays as twoinsertpsand does not regress.Larger vectors are still split into per-128-bit-lane
Createnodes before reaching the base case, so this applies per lane and a fully constant lane collapses to aCNS_VECcombined viaWithUpper/WithLower.SuperPMI asmdiffs (windows x64, 2,596,354 contexts): -806 bytes overall, no regressions.
Notable improvements:
System.Buffers.Text.Base64Helper:Avx2Encode-290 (-25.33%)GitHub_43569.Program:Vector128_Create_byte-155 (-42.12%)GitHub_43569.Program:Vector256_Create_float/Vector128_Create_float-21 eachCorrectness was validated across all base types and vector sizes with non-constant elements at various positions, under Checked asserts, across ISA configs (AVX512/AVX2/AVX off) and
JitStress=2.Note
This PR description was drafted with the assistance of GitHub Copilot.