Runtime: Math/MathF.FusedMultiplyAdd/FusedMultiplyAdd not treated as intrinsics on ARM64

[jkotas]

Missing ARM64 handling at

https://github.com/dotnet/runtime/blob/4f38b755fd8953830b58b4bc42943c57feeec419/src/coreclr/src/jit/importer.cpp#L4198

[EgorBo]

I guess that code can be just enabled for arm with the only change NI_FMA_MultiplyAddScalar to NI_AdvSimd_FusedMultiplyAddScalar and it will work.

Optional: https://github.com/dotnet/coreclr/pull/27060 for AdvSimd.

[EgorBo]

Something like this: https://github.com/EgorBo/runtime-1/commit/0074845aceca270ca815b9668bfc32b33956d706

upd: ah, not that simple, NI_Vector128_CreateScalarUnsafe is xarch only atm

[EgorBo]

@echesakovMSFT @tannergooding

looks like AdvSimd.FusedMultiplyAddScalar is broken

[MethodImpl(MethodImplOptions.NoInlining)]
static float Test(float x, float y, float z)
{
    Vector64<float> vecX = Vector64.CreateScalarUnsafe(x);
    Vector64<float> vecY = Vector64.CreateScalarUnsafe(y);
    Vector64<float> vecZ = Vector64.CreateScalarUnsafe(z);
    Vector64<float> rez  = AdvSimd.FusedMultiplyAddScalar(vecX, vecY, vecZ);
    return rez.ToScalar();
}

Console.WriteLine(Test(2,4,6)) prints 26 but should be 14 (wrong order of operands here?)

Also, double version of this method just crashes:

[MethodImpl(MethodImplOptions.NoInlining)]
static double Test4(double x, double y, double z)
{
    Vector64<double> vecX = Vector64.Create(x);
    Vector64<double> vecY = Vector64.Create(y);
    Vector64<double> vecZ = Vector64.Create(z);
    Vector64<double> rez  = AdvSimd.FusedMultiplyAddScalar(vecX, vecY, vecZ);
    return rez.ToScalar();
}

Assert failure(PID 11202 [0x00002bc2], Thread: 11202 [0x2bc2]): Assertion failed '!"Didn't find a class handle for simdType"' in 'Repro:Test4(double,double,double):double' during 'Importation' (IL size 33)

    File: /home/egorb/runtime/src/coreclr/src/jit/hwintrinsic.cpp Line: 218

[tannergooding]

The operand order difference is expected. ARM64 takes them as addend, left, right rather then left, right, addend

[EgorBo]

@tannergooding I changed the order to GetEmitter()->emitIns_R_R_R_R(ins, emitSize, targetReg, op1Reg, op2Reg, op3Reg); re-compiled and got expected 14 for my sample

[EgorBo]

@tannergooding or are you saying I should reverse them when I call the C# API?

[tannergooding]

When constructing the intrinsic node, you should order them as op3, op1, op2, rather than as op1, op2, op3

[echesakovMSFT]

I have a draft change in my private branch https://github.com/echesakovMSFT/runtime/tree/Arm64-Math-MathF-FusedMultiplyAdd that I did as a background task a while ago. I can resurrect the change and finish it if this is needed for 5.0.

[EgorBo]

Sorry guys, I was on a out-of-date branch, master works fine for me.

[echesakovMSFT]

@EgorBo Are you gonna continue working on this or you want me to take over?

@jkotas @tannergooding Do you think this work belong to 5.0?

[tannergooding]

It should be a trivial change that helps boost perf for functions that use it, I imagine taking it would be worthwhile l.

[EgorBo]

Well, actually, no the both problems I listed ^ still here on fresh master.
Here is my draft: https://github.com/dotnet/runtime/compare/master...EgorBo:arm64-math-fma?expand=1
it works fine for float:

using System;
using System.Runtime.CompilerServices;

class Repro
{
    static void Main()
    {
        Console.WriteLine(Test_f(2,4,6));
        //Console.WriteLine(Test_d(2,4,6));
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static float Test_f(float x, float y, float z)
    {
        return MathF.FusedMultiplyAdd(x, y, z);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static double Test_d(double x, double y, double z)
    {
        return Math.FusedMultiplyAdd(x, y, z);
    }
}

Codegen for Test_f:

G_M29070_IG01:
        A9BF7BFD          stp     fp, lr, [sp,#-16]!
        910003FD          mov     fp, sp
                                                ;; bbWeight=1    PerfScore 1.50
G_M29070_IG02:
        1E204030          fmov    s16, s1
        1E204051          fmov    s17, s2
        1F004220          fmadd   s0, s17, s0, s16
                                                ;; bbWeight=1    PerfScore 4.00
G_M29070_IG03:
        A8C17BFD          ldp     fp, lr, [sp],#16
        D65F03C0          ret     lr
                                                ;; bbWeight=1    PerfScore 2.00

; Total bytes of code 28, prolog size 8, PerfScore 10.30, (MethodHash=16048e71) for method Repro:Test_f(float,float,float):float
; ============================================================

However, it crashes for Test_d

Assert failure(PID 4276 [0x000010b4], Thread: 4276 [0x10b4]): Assertion failed 'ins != INS_invalid' in 'Repro:Test_d(double,double,double):double' during 'Generate code' (IL size 9)

    File: /home/egorb/runtime/src/coreclr/src/jit/hwintrinsiccodegenarm64.cpp Line: 485

UPD oh, maybe NI_Vector64_CreateScalarUnsafe just doesn't support double and should be just NI_Vector64_Create
@echesakovMSFT feel free to take over 🙂 I see your branch also handles FMA variations (depending on NEG).

[echesakovMSFT]

However, it crashes for Test_d

Let me take a look

[AndyAyersMS]

Marking as 5.0.

[EgorBo]

@echesakovMSFT ah, nvm, fixed via NI_Vector64_Create for double.
Full PR: https://github.com/dotnet/runtime/compare/master...EgorBo:arm64-math-fma?expand=1 now works for both float and double 🙂