Skip to content
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

Arm64/Sve: Validation for movprfx in emitter #105514

Closed
kunalspathak opened this issue Jul 25, 2024 · 4 comments · Fixed by #106184
Closed

Arm64/Sve: Validation for movprfx in emitter #105514

kunalspathak opened this issue Jul 25, 2024 · 4 comments · Fixed by #106184
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@kunalspathak
Copy link
Member

As noted in #100743 (comment) . Everytime we emit SVE instruction, we should check the last emitted instruction and if it is movprfx, then validate the things required for movprfx placement.

image

@kunalspathak kunalspathak added this to the 9.0.0 milestone Jul 25, 2024
@kunalspathak kunalspathak added arm-sve Work related to arm64 SVE/SVE2 support area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Reflection.Emit labels Jul 25, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@a74nh
Copy link
Contributor

a74nh commented Jul 30, 2024

priority:2 for RC1 snap : This is a functional correctness issue - if the rules are violated then the use is UNPREDICTABLE.

@a74nh a74nh added the Priority:2 Work that is important, but not critical for the release label Jul 30, 2024
@mikabl-arm
Copy link
Contributor

Everytime we emit SVE instruction, we should check the last emitted instruction ...

Shouldn't this be done for any instruction and not just SVE instructions?

@a74nh a74nh self-assigned this Aug 5, 2024
@a74nh
Copy link
Contributor

a74nh commented Aug 6, 2024

Thoughts:

I'll be adding a new function, something like emitInsSanityCheckWithPrevious()

If this check fails what do we do? Is this just an assert and exit?

We need the previous instruction. Calling emitNewInstrCns() (and friends) will set emitLastIns to this new instruction.

One way is to add emitPrevIns tracking (I think I remember us trying to add that with the peephole optimisation). Advantage now is we can call the new checks from emitInsSanityCheck().

If we don't have tracking, then we need to do the checking before creating the new instruction, manually passing in the id, reg1, reg2 etc etc (just like how ReplaceLdrStrWithPairInstr() works). Now we need to call our new check function before calling emitNewInstrCns() and this needs to be done in every Arm64 emit function.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants