JIT: Added four SVE CreateBreak* APIs
#104184
Conversation
|
Note regarding the |
1 similar comment
|
Note regarding the |
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
|
@dotnet/arm64-contrib @kunalspathak this is ready. stress test results:
|
| HARDWARE_INTRINSIC(Sve, CreateBreakAfterMask, -1, 2, true, {INS_sve_brka, INS_sve_brka, INS_sve_brka, INS_sve_brka, INS_sve_brka, INS_sve_brka, INS_sve_brka, INS_sve_brka, INS_invalid, INS_invalid}, HW_Category_SIMD, HW_Flag_Scalable|HW_Flag_ExplicitMaskedOperation|HW_Flag_ReturnsPerElementMask|HW_Flag_SpecialCodeGen) | ||
| HARDWARE_INTRINSIC(Sve, CreateBreakAfterPropagateMask, -1, 3, true, {INS_sve_brkpa, INS_sve_brkpa, INS_sve_brkpa, INS_sve_brkpa, INS_sve_brkpa, INS_sve_brkpa, INS_sve_brkpa, INS_sve_brkpa, INS_invalid, INS_invalid}, HW_Category_SIMD, HW_Flag_Scalable|HW_Flag_ExplicitMaskedOperation|HW_Flag_ReturnsPerElementMask|HW_Flag_SpecialCodeGen) | ||
| HARDWARE_INTRINSIC(Sve, CreateBreakBeforeMask, -1, 2, true, {INS_sve_brkb, INS_sve_brkb, INS_sve_brkb, INS_sve_brkb, INS_sve_brkb, INS_sve_brkb, INS_sve_brkb, INS_sve_brkb, INS_invalid, INS_invalid}, HW_Category_SIMD, HW_Flag_Scalable|HW_Flag_ExplicitMaskedOperation|HW_Flag_ReturnsPerElementMask|HW_Flag_SpecialCodeGen) | ||
| HARDWARE_INTRINSIC(Sve, CreateBreakBeforePropagateMask, -1, 3, true, {INS_sve_brkpb, INS_sve_brkpb, INS_sve_brkpb, INS_sve_brkpb, INS_sve_brkpb, INS_sve_brkpb, INS_sve_brkpb, INS_sve_brkpb, INS_invalid, INS_invalid}, HW_Category_SIMD, HW_Flag_Scalable|HW_Flag_ExplicitMaskedOperation|HW_Flag_ReturnsPerElementMask|HW_Flag_SpecialCodeGen) |
There was a problem hiding this comment.
wondering why you did not include CreateBreakPropagateMask?
There was a problem hiding this comment.
I think that API needs a closer look as its signature doesn't correspond well to its instruction based on its parameter names and parameter count.
| case NI_Sve_CreateBreakAfterMask: | ||
| case NI_Sve_CreateBreakBeforeMask: | ||
| { | ||
| GetEmitter()->emitInsSve_R_R_R(ins, emitSize, targetReg, op1Reg, op2Reg, INS_OPTS_SCALABLE_B); |
There was a problem hiding this comment.
For BRKB and BRKA, if we do not pass insScalableOpts, it is treated as BRK* Pd.B, Pg/Z, Pn.B. How do we generate the merge variant? If we want to support one of those, should merge be the default one? Accordingly we should also update the summary docs and remove the instruction that we are not supporting. We also need to think about what if all targetReg, op1Reg and op2Reg end up to same register or two of them have same register, does it change the operation of Pg/M vs. Pg/Z. Lastly, we should add test coverage (if not there currently) where we pass same value to mask and srcMask to simulate such behavior.
There was a problem hiding this comment.
When looking at the original generated API, there were two C intrinsic variants that it listed:
M variant https://dougallj.github.io/asil/doc/brka_p_p_p_m_8.html
Z variant https://dougallj.github.io/asil/doc/brka_p_p_p_z_8.html
Based on the API signature of our intrinsic, it matches exactly to the Z variant - so naturally I think that's most likely the correct one. I figured the M variant that was listed is an artifact of the API generation; as other APIs have before. This also leads me to believe that exposing each variant needs to have their own API.
There was a problem hiding this comment.
I figured the M variant that was listed is an artifact of the API generation
Yes, the API generation will have just blindly put them together.
This also leads me to believe that exposing each variant needs to have their own API.
We can we replicate the zero version using:
CndSel(mask, CreateBreakAfterMask(mask, op1), zero)
And then in lowering or somewhere, optimise that to a single BRKA with zeroing.
(Or, if I'm wrong and that doesn't work, then yes, ideally add a extra API)
Doesn't need doing now, but could you raise a ticket please.
There was a problem hiding this comment.
We can we replicate the zero version using:
Do you mean "merge"? The API is the zero version currently
There was a problem hiding this comment.
could you raise a ticket please.
+1 on that if we decide to skip a variant of the instruction.
There was a problem hiding this comment.
I agree with just keeping "Z" version. Just update the summary docs.
|
@kunalspathak Fixed the comments and merged with main, so it's ready |
|
/ba-g failures are unrelated. superpmi diff failures are related to timeout. |
Contributes to #99957
Adds: