Fix incorrect codegen for SVE Scatter*With*Offsets* #119712
Conversation
Currently, offsets are incorrectly treated as indices which is leading to incorrect code being emitted. e.g., `ScatterWithByteOffsets<long>` emits `ST1D Zdata.D, Pg, [Xbase, Zoffsets.D, lsl #3]` instead of, `ST1D Zdata.D, Pg, [Xbase, Zoffsets.D]`
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
a74nh
added
the
Priority:1
|
This is an error in our implementation for .NET9. As such, I'm marking this as .NET10 and high priority. |
| case NI_Sve_Scatter16BitWithByteOffsetsNarrowing: | ||
| case NI_Sve_Scatter32BitWithByteOffsetsNarrowing: | ||
| case NI_Sve_Scatter8BitWithByteOffsetsNarrowing: | ||
| case NI_Sve_ScatterWithByteOffsets: |
There was a problem hiding this comment.
Were there any other instructions (such as gather) which differed between indices vs offsets like these ones?
I know we've reviewed/approved a few groups, but I don't know which are actually implemented today.
There was a problem hiding this comment.
I found the following APIs that are incorrectly treating offsets as indices and would need a similar fix.
GatherVectorInt16WithByteOffsetsSignExtend (long/ulong combinations)
GatherVectorInt32WithByteOffsetsSignExtend (long/ulong)
GatherVectorInt16WithByteOffsetsSignExtendFirstFaulting (long/ulong)
GatherVectorInt32WithByteOffsetsSignExtendFirstFaulting (long/ulong)
GatherVectorUInt16WithByteOffsetsZeroExtend (long/ulong)
GatherVectorUInt16WithByteOffsetsZeroExtendFirstFaulting (long/ulong)
GatherVectorUInt32WithByteOffsetsZeroExtend(long/ulong)
Do you recommend a separate PR for the above or should put in the current one?
There was a problem hiding this comment.
Since its the same general issue and we're looking at backporting, I'd put them in the same PR
There was a problem hiding this comment.
We should make sure these ones still get a fix done and also included in the backport
|
@tannergooding, we will backport this to .NET 10. Do we need to backport to .NET 9? |
|
@SwapnilGaikwad, please check test failures. |
|
The feature remains experimental (and requires explicit user opt-in) in both .NET 9 and .NET 10 I think it would be fine to go into .NET 10, being an LTS and a known issue prior to RTM. I don't think we need to also backport to .NET 9 without some explicit customer report, particularly given .NET 9 is EoL in May of next year. |
|
Agreed. @EgorBo, let's backport to .NET 10 only. |
Failures seem unrelated to the PR. The changes are SVE only and the failures are for x64. |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17771937971 |
I presume we won't merge this PR into main, right? |
|
Ah, you meant no .NET 9.0, got it |
|
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@EgorBo Are these Fuzzlyn error results expected? When running latest HEAD, Meanwhile |
|
@a74nh it looks like these failures reproduce on Main too on that pipeline, although, they're SVE related it seems. I guess we can merge the PR |
|
/ba-g "timeouts" |
|
Oops, didn't realize there is a pending work on this one given @tannergooding comments. let's address that and file a backport with those? |
Currently, offsets are incorrectly treated as indices which is leading to incorrect code being emitted.
e.g.,
ScatterWithByteOffsets<long>emitsST1D Zdata.D, Pg, [Xbase, Zoffsets.D, lsl #3]instead of,
ST1D Zdata.D, Pg, [Xbase, Zoffsets.D]@dotnet/arm64-contrib @a74nh