-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Arm64 SVE: Add mask to nonfaulting loads #117606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes dotnet#108234 SveLoadNonFaultingMaskedUnOpTest.template is a mixture of SveLoadNonFaultingUnOpTest.template and SveLoadVectorMaskedTest.template
@amanasifkhalid @dotnet/arm64-contrib |
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
I'm guessing this counts as a breaking change, since we're diverging from .NET 9's API surface -- not sure if there's something else we need to do to suppress ApiCompat in CI |
I forgot to add the apicompact change. Seems to have fixed it now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I looked at a few other breaking SVE changes, and since SVE support was experimental in .NET 9, I think we're fine to skip the usual documentation. @gewarren should we track this renaming like dotnet/dotnet-api-docs#11427 does? The technical reason for this change might warrant an explanation to early SVE adopters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a74nh looks like there's some asserts to sort out. From SPMI:
ISSUE: <ASSERT> #119969 /Users/runner/work/1/s/src/coreclr/jit/gentree.h (6080) - Assertion failed 'actualIndex < m_operandCount' in 'JIT.HardwareIntrinsics.Arm._Sve.LoadNonFaultingUnaryOpTest__Sve_LoadVectorInt16NonFaultingSignExtendToUInt32_uint:ConditionalSelectScenario_FalseValue(System.Numerics.Vector`1[uint],ptr,System.Numerics.Vector`1[uint]):this' during 'Importation' (IL size 60; hash 0xbe66dabb; MinOpts)
I wonder if this is spmi running the existing versions of the tests. Which mean it's constructing an import graph which is no longer valid for this version of the jit (ie LoadVectorInt16NonFaultingSignExtendToUInt32 only has one arg instead of two). Will check... |
Yes, it's trying to import an API call that doesn't exist anymore:
I don't think there is anything we can do here? I ideally don't want to add support in the jit for this only then to remove it after spmi has been regenerated. |
Right, thanks for taking a look. I think you'll want to bump the JIT-EE GUID so we can kick off a new collection once this is merged. |
@dotnet/jit-contrib FYI |
Right, makes sense. Updated. |
Build errors are all "send to helix" issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
/ba-g android-x64 CoreCLR build stuck |
@amanasifkhalid This is not a rename, but an additional parameter, correct? If so, to me it doesn't need to be tracked like 11427. But we have documented preview-to-preview breaking changes in the past, which is sort of similar to an experimental feature since previews aren't supported for production (I think). So maybe it should just be documented as a regular breaking change by filling out https://github.com/dotnet/docs/issues > .NET breaking change? |
#108234 was raised before this PR to track getting approval by the libraries team. I'm not sure if that counts, or if you're asking for something else. |
That's correct.
Typically, the PR author of a breaking change opens an issue in dotnet/docs describing the breaking change to ensure it is documented upon product release. It's something we definitely have to do when features we've previously supported are affected. SVE support was experimental (and thus subject to change) in .NET 9, but I think we ought to document this, considering it's one of few changes (AFAIK) to the SVE API surface between .NET 9 and 10. @a74nh would you be able to open the breaking change issue? |
Done: dotnet/docs#47439 |
Removing |
Fixes #108234
SveLoadNonFaultingMaskedUnOpTest.template is a mixture of SveLoadNonFaultingUnOpTest.template and
SveLoadVectorMaskedTest.template