Conversation
…Simd.PlatformNotSupported.cs
…imd.cs AdvSimd.PlatformNotSupported.cs
…rmNotSupported.cs
…ormNotSupported.cs
…Simd.PlatformNotSupported.cs
…trinsiccodegenarm64.cpp
…ee in lsraarm64.cpp
| case NI_AdvSimd_Arm64_FusedMultiplyAdd: | ||
| case NI_AdvSimd_Arm64_FusedMultiplySubtract: | ||
| case NI_AdvSimd_MultiplyAdd: | ||
| case NI_AdvSimd_MultiplySubtract: |
There was a problem hiding this comment.
It might be worth checking the x86 handling here. Given it is three operands and doing a * b + c there was special handling added such that a and b could still be considered commutative (etc).
x86 also has a (I believe in lowering) optimization for converting things like FusedMultiplyAdd(a, b, -c) to a FusedMultiplySubtract(a, b, c)
There was a problem hiding this comment.
It might be worth checking the x86 handling here. Given it is three operands and doing a * b + c there was special handling added such that a and b could still be considered commutative (etc).
Yes, there is a special handling on x86 but, as far as I understand, it takes into account whether any of a, b or c operands is a memory operand and whether the upper bits of operands are preserved.
On arm64 there is only one form for each of the instructions (e.g. FMLA <Vc>, <Va>, <Vb>) so I am not sure what else we can do here?
x86 also has a (I believe in lowering) optimization for converting things like FusedMultiplyAdd(a, b, -c) to a FusedMultiplySubtract(a, b, c)
Yes, @EgorBo added this optimization in dotnet/coreclr#27060. I was planning working on these later but not as part of this PR.
There was a problem hiding this comment.
Some of the x86 handling is because you can have op1 * op3 + op2 or op2 * op1 + op3 or op2 * op3 + op1. That handling is x86 specific.
There is also handling for allowing a * b + c or b * a + c since the multiplication is commutative. That handling also applies to ARM.
I was planning working on these later but not as part of this PR.
Sounds good to me, as long as it is being tracked.
There was a problem hiding this comment.
Ah, I think I see.
Since it takes 4 registers (a destination and three inputs) there is no need for it to be commutative. I had misread and thought it was 3 inputs where one was also the destination.
There was a problem hiding this comment.
The vector version (FMLA) looks interesting however, since it does only take 3 registers where the accumulator is also the destination
There was a problem hiding this comment.
What instructions are we talking about here?
-
FMADD, FNMADD, FMSUB, FNMSUB take four registers - destination and three source registers - these instructions are emitted during FusedMultiplyAddScalar, FusedMultiplyAddNegatedScalar, FusedMultiplySubtractScalar, FusedMultiplySubtractNegatedScalar. LSRA does not have any special logic for those.
-
On other hand, FMLA, FMLS, MLA, MLS take only three - destination and incoming accumulator operand register and two multipliers' registers. Those are used for vector forms of FusedMultiplyAdd, FusedMultiplySubract, MultiplyAdd, MultiplySubtract. The logic in lsraarm64.cpp applies only to these instructions/intrinsics. My question how we can use commutativity of operands in this particular case?
There was a problem hiding this comment.
I had been looking at FMLA initially as it only takes three and I missed that FMADD took four.
I had also thought that the src/dest operand was one of the multipliers and so commutativity mattered.
| @@ -210,6 +210,18 @@ public abstract partial class AdvSimd : System.Runtime.Intrinsics.Arm.ArmBase | |||
| public static System.Runtime.Intrinsics.Vector64<uint> CompareTest(System.Runtime.Intrinsics.Vector64<uint> left, System.Runtime.Intrinsics.Vector64<uint> right) { throw null; } | |||
| public static System.Runtime.Intrinsics.Vector64<double> DivideScalar(System.Runtime.Intrinsics.Vector64<double> left, System.Runtime.Intrinsics.Vector64<double> right) { throw null; } | |||
| public static System.Runtime.Intrinsics.Vector64<float> DivideScalar(System.Runtime.Intrinsics.Vector64<float> left, System.Runtime.Intrinsics.Vector64<float> right) { throw null; } | |||
| public static System.Runtime.Intrinsics.Vector128<float> FusedMultiplyAdd(System.Runtime.Intrinsics.Vector128<float> acc, System.Runtime.Intrinsics.Vector128<float> left, System.Runtime.Intrinsics.Vector128<float> right) { throw null; } | |||
There was a problem hiding this comment.
We may want to discuss the ordering in API review.
The scalar versions are FMADD <Sd>, <Sn>, <Sm>, <Sa>, where it does Sd = Sa + Sn * Sm (registers ordered dest, left, right, acc)
The vector versions are FMLA <Vd>, <Vn>, <Vm> where it does Vd = Vd + Vn * Vm (registers ordered dest/acc, left, right as you have here)
x86 intrinsics and the public Math.FusedMultiplyAdd methods are all dest, left, right, acc
…t-Multiply-Add-Subtract
This implements the fused multiply-add and multiply-subtract intrincsics and multiply-add multiply-subtract for integer types.
Note that this PR doesn't have an implementation for
FMLA <Vd>.<T>, <Vn>.<T>, <Vm>.<Ts>[<index>]- this will be done in a separate PR.Part of #24794