Uh oh!
There was an error while loading. Please reload this page.
Fix Vector.Dot on arm64 - #133215
Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 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 |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped to ARM64 lowering, matches the reported failure mode, and includes targeted regression coverage to prevent recurrence.
Pull request overview
Fixes ARM64 codegen for Vector.Dot when Morph drops Create(ToScalar(Dot(...))) and the remaining Dot value is subsequently consumed as a vector (requiring a broadcast rather than lane-0-only semantics).
Changes:
- Update ARM64 lowering for
NI_Vector_Dotto insert an explicit broadcast when theDotresult is not consumed byToScalar. - Add a JIT regression test covering the motivating
Vector128.IndexOf(..., Vector.Dot(...))pattern and additionalVector64/Vector128.Create(Dot(...))broadcast cases. - Wire the new regression test into the
Regression_ro_2.csprojcompile items.
File summaries
| File | Description |
|---|---|
| src/coreclr/jit/lowerarmarch.cpp | Inserts a broadcast during ARM64 lowering of Vector.Dot when the use is not ToScalar, preserving expected “scalar splat” behavior. |
| src/tests/JIT/Regression/JitBlue/Runtime_132800/Runtime_132800.cs | Adds a regression test validating correct broadcast semantics for Vector.Dot/Vector64.Dot/Vector128.Dot patterns on AdvSimd. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new Runtime_132800 test file in the regression project build. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| // Morph 'fgOptimizeHWIntrinsic' transforms 'Create(ToScalar(Dot('->'Dot(', | ||
| // so a DotProduct not consumed as a scalar value needs an explicit broadcast | ||
| LIR::Use use; | ||
| bool needsBroadcast = BlockRange().TryGetUse(node, &use) && !use.User()->OperIsHWIntrinsic(NI_Vector_ToScalar); |
There was a problem hiding this comment.
🟢 Approval recommended
The ARM64 lowering change is narrowly scoped, matches existing intrinsic patterns, and is backed by a targeted regression test exercising the problematic consumption shape.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
| // Morph 'fgOptimizeHWIntrinsic' transforms 'Create(ToScalar(Dot(...' into 'Dot('..., | ||
| // so a DotProduct not consumed as a scalar value needs an explicit broadcast | ||
| LIR::Use use; | ||
| bool needsBroadcast = BlockRange().TryGetUse(node, &use) && !use.User()->OperIsHWIntrinsic(NI_Vector_ToScalar); |
There was a problem hiding this comment.
This primarily differs from xarch in the type of check because you're wanting to avoid the DUP if its unnecessary, right?
xarch notably distinguishes this based on varTypeIsSIMD(node-gtType) instead and uses that to decide to insert ToScalar because NI_Vector_Dot is allowed to be either TYP_FLOAT/DOUBLE or TYP_SIMD. But it always "broadcasts" because it always needs to reduce and its simpler to go that route there.
I am, however, somewhat considering whether the IR shape differing between xarch and arm64 needs more explicit documentation and whether or not its potentially problematic for other phases (divergence, missing opts, etc).
There was a problem hiding this comment.
I think in the worst case here, we might at the very least be unnecessarily generating DUP for a TYP_FLOAT typed SIMD, which I don't believe is harmful just something that isn't strictly needed and may cause nuance/confusion
There was a problem hiding this comment.
It does look like LowerHWIntrinsicDot on xarch already asserts "varTypeISSIMD(Node)", unless you were talking about something else
anyway, the extra DUP here should only be along the integral/ADDV lowering path
tannergooding
left a comment
There was a problem hiding this comment.
Changes look correct to me, but do have a question on the general handling as compared to xarch and whether its potentially missing any cases due to the return type being allowed to be scalar or simd
Morph removes Create+ToScalar from trees like
Create(ToScalar(Dot(...Leaving something like this, where there's assumed to be an implicit broadcast after the
Dot:ARM64 currently lowers Vector.Dot into an AddAccross that writes its result into lane 0 of vector reg and zeros other lanes.
To fix ideally we'd instead do the morph transform during lower. However, by that point CSE had replaced the
ToScalar(Dot(...which causes bad diffs on x64 in the motivating example.So instead, leave the morph transform and just insert a broadcast during arm64 lower if the use is not a ToScalar. Gives 0 diffs.
fixes#132800