ARM64-SVE: Add AddRotateComplex, MultiplyAddRotateComplex
#104926
Conversation
dotnet-issue-labeler
bot
added
area-CodeGen-coreclr
|
Note regarding the |
1 similar comment
|
Note regarding the |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| { | ||
| for (int i = 0; i < op1.Length; i += 2) | ||
| { | ||
| if (rot == 0) |
There was a problem hiding this comment.
likewise here - from https://developer.arm.com/architectures/instruction-sets/intrinsics/svcadd[_f64]_m
can we use real and img and then calculate the result to make it clear.
| case 4: | ||
| assert(intrinEmbMask.op4 != nullptr); | ||
| assert(HWIntrinsicInfo::HasImmediateOperand(intrinEmbMask.id)); | ||
| FALLTHROUGH; |
There was a problem hiding this comment.
can you also save embMaskOp4Reg and then later, check if it is REG_NA because it was contained?
No need for assert(HWIntrinsicInfo::HasImmediateOperand(intrinEmbMask.id)); at this point.
| case 3: | ||
| case 4: |
There was a problem hiding this comment.
hhm, any reason why we are combining 3 and 4? This can lead to accidently not handling things correctly in future.
There was a problem hiding this comment.
If the intent is to share the following code, then we should extract it out I think in a common method and call it for case 3 and 4.
if (intrin.op2->IsVectorZero())
{
...
}
else
{
...
}There was a problem hiding this comment.
That was my intent; I'll do that
| { | ||
| for (int i = 0; i < op1.Length; i += 2) | ||
| { | ||
| if (rot == 0) |
|
@kunalspathak thanks for the review! Stress tests are still passing. |
| case NI_Sve_ShiftRightArithmeticForDivide: | ||
| assert(embHasImmediateOperand); | ||
| assert(numArgs == 2); | ||
| if (!embOp2Node->Op(2)->isContainedIntOrIImmed()) |
There was a problem hiding this comment.
nit for future PR - we can have bool needInternalRegister that gets set and then after the switch-case just check and do buildInternalIntRegisterDefForNode(embOp2Node); if it is true.
There was a problem hiding this comment.
That sounds better -- I'll include that in my next PR.
Part of #99957. Based on discussion with Tanner here, we decided the
rotationvalue should be passed in its encoded form (0, 1, etc.) rather than as an angle to simplify the JIT's implementation; in the future API review round, we'll changerotationto be an enum to simplify the API's usage. This meant I had to tweak the emitter and its corresponding SVE unit tests. I reran them, and after disabling a few unit tests that are NYI, they all emit without hitting any asserts.I also tweaked
_SveImmBinaryOpTestTemplate.templateand_SveImmTernOpTestTemplate.templatewhile I was here to address #104804 and #104809, since relatively few tests use these templates. For the APIs using these templates, including the newly-added ones, the stress tests are passing.@dotnet/arm64-contrib PTAL, thanks!