JIT: Added Sve.CreateBreakPropagateMask #104704

TIHan · 2024-07-11T01:44:58Z

Contributes to #99957

Adds

CreateBreakPropagateMask

dotnet-issue-labeler · 2024-07-11T01:45:03Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2024-07-11T01:45:04Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-policy-service · 2024-07-11T01:45:27Z

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

TIHan · 2024-07-11T19:42:09Z

@dotnet/arm64-contrib @kunalspathak @tannergooding this is ready.
Ran stress tester 3 times and it all looks good:

===================Running default===================
------------------- {} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_byte() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_ushort() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_uint() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_ulong() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_sbyte() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_short() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_int() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_long() : 31
===================Running jitstress===================
------------------- {'JitMinOpts': '1'} -------------------
------------------- {'JitStress': '1'} -------------------
------------------- {'JitStress': '2'} -------------------
------------------- {'JitStress': '1', 'TieredCompilation': '1'} -------------------
------------------- {'JitStress': '2', 'TieredCompilation': '1'} -------------------
------------------- {'TailcallStress': '1'} -------------------
------------------- {'ReadyToRun': '0'} -------------------
===================Running jitstressregs===================
------------------- {'JitStressRegs': '1'} -------------------
------------------- {'JitStressRegs': '2'} -------------------
------------------- {'JitStressRegs': '3'} -------------------
------------------- {'JitStressRegs': '4'} -------------------
------------------- {'JitStressRegs': '8'} -------------------
------------------- {'JitStressRegs': '0x10'} -------------------
------------------- {'JitStressRegs': '0x80'} -------------------
------------------- {'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStressRegs': '0x2000'} -------------------
===================Running jitstress2-jitstressregs===================
------------------- {'JitStress': '2', 'JitStressRegs': '1'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '2'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '3'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '4'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '8'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x10'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x80'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x2000'} -------------------

src/coreclr/jit/hwintrinsic.cpp

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

src/coreclr/jit/lsraarm64.cpp

src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs

tannergooding · 2024-07-11T23:55:13Z

Overall this looks good/correct to me.

There's a couple small nits, some callouts around pre-existing code that doesn't look to be quite correct, and then one real case that I'm not sure is correctly handled (as op3 should only ever be a contained zero for CreateBreakPropagateMask).

TIHan · 2024-07-12T21:19:47Z

@tannergooding Did feedback.

Stress results:

===================Running default===================
------------------- {} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_byte() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_ushort() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_uint() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_ulong() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_sbyte() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_short() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_int() : 31
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateBreakPropagateMask_long() : 31
===================Running jitstress===================
------------------- {'JitMinOpts': '1'} -------------------
------------------- {'JitStress': '1'} -------------------
------------------- {'JitStress': '2'} -------------------
------------------- {'JitStress': '1', 'TieredCompilation': '1'} -------------------
------------------- {'JitStress': '2', 'TieredCompilation': '1'} -------------------
------------------- {'TailcallStress': '1'} -------------------
------------------- {'ReadyToRun': '0'} -------------------
===================Running jitstressregs===================
------------------- {'JitStressRegs': '1'} -------------------
------------------- {'JitStressRegs': '2'} -------------------
------------------- {'JitStressRegs': '3'} -------------------
------------------- {'JitStressRegs': '4'} -------------------
------------------- {'JitStressRegs': '8'} -------------------
------------------- {'JitStressRegs': '0x10'} -------------------
------------------- {'JitStressRegs': '0x80'} -------------------
------------------- {'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStressRegs': '0x2000'} -------------------
===================Running jitstress2-jitstressregs===================
------------------- {'JitStress': '2', 'JitStressRegs': '1'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '2'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '3'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '4'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '8'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x10'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x80'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x2000'} -------------------

kunalspathak · 2024-07-13T14:35:21Z

I might have asked this earlier somewhere, but BRKN operates on .B and returns .B, so shouldn't we just have one API:

Vector<byte> CreateBreakPropagateMask(Vector<byte> totalMask, Vector<byte> fromMask)

Currently with other overloads, the instruction will calculate the result in byte lanes, but we will (mis)interpret it as short, uint, etc.?

tannergooding · 2024-07-13T19:25:37Z

so shouldn't we just have one API:

I elaborated on this the other place you asked (see #104502 (comment)). Having one for each type is correct and still fits into how predicates actually work.

For x64 we have a kmask register that is always 64-bits and we then take the lowest n-bits such as:

Vector512 - 64-bit mask, no ignored bits
Vector512 - 32-bit mask, ignored upper 32-bits
Vector256 - 32-bit mask, ignored upper 32-bits
Vector512 - 16-bit mask, ignored upper 48-bits
Vector256 - 16-bit mask, ignored upper 48-bits
Vector128 - 16-bit mask, ignored upper 48-bits
Vector512 - 8-bit mask, ignored upper 56-bits
Vector256 - 8-bit mask, ignored upper 56-bits
Vector128 - 8-bit mask, ignored upper 56-bits
Vector256 - 4-bit mask, ignored upper 60-bits
Vector128 - 4-bit mask, ignored upper 60-bits
Vector128 - 2-bit mask, ignored upper 62-bits

So for x64, a mask of Vector128<float> being 4-bits has to actually convert to get a valid Vector128<byte> mask which needs 16-bits; so there are conversion instructions and suffixes that say how many bits to read.

While for Arm64 we have a pg register that is always sizeof(Vector<T>)-bits (so if SVE registers are 128-bits, then its size is 16-bytes and therefore there are 16-bits in the pg register) we then take bits as:

Vector - take every bit
Vector - take every other bit
Vector - take every fourth bit
Vector - take every eighth bit

So there is no conversion required between these to get something "usable". There may, however, be some normalization consideration in some edge cases.

For an instruction like this the mask returned despite being .B is actually universal because it's simply indicating what bytes were valid and that cleanly maps to which elements end up as valid. It then works as expected when you're matching the relevant load types with the type you read the FFR as

kunalspathak

Added some questions.

src/coreclr/jit/hwintrinsic.cpp

kunalspathak · 2024-07-13T14:41:19Z

src/coreclr/jit/hwintrinsiclistarm64sve.h

@@ -58,6 +58,7 @@ HARDWARE_INTRINSIC(Sve,           CreateBreakAfterMask,
 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)
+HARDWARE_INTRINSIC(Sve,           CreateBreakPropagateMask,                                         -1,      -1,     false, {INS_sve_brkn,       INS_sve_brkn,       INS_sve_brkn,       INS_sve_brkn,       INS_sve_brkn,       INS_sve_brkn,       INS_sve_brkn,       INS_sve_brkn,       INS_invalid,        INS_invalid},     HW_Category_SIMD,                  HW_Flag_Scalable|HW_Flag_EmbeddedMaskedOperation|HW_Flag_ReturnsPerElementMask|HW_Flag_HasRMWSemantics)


Should't this have HW_Flag_ExplicitMaskedOperation too?

From the above comment, I can't combine ExplicitMaskedOperation and EmbeddedMaskedOperation.

kunalspathak · 2024-07-14T05:00:23Z

src/coreclr/jit/lsraarm64.cpp

+                    assert(intrin.op3->IsVectorZero());
+                    tgtPrefUse = BuildUse(embOp2Node->Op(2));
+                    srcCount += 1;
+                    srcCount += BuildDelayFreeUses(embOp2Node->Op(1), embOp2Node->Op(2));


I am little worried about the handling of this. Can you share the JitDump of one of the simpler repro scenario?

Sure, I'll get a JitDump of the LoadBasicScenario.

What specifically are you worried about?

here we are not building refpositions in order. Basically it should be for Op(1), Op(2) and then Op(3), but here, we are doing Op(2), Op(1) and Op(3).

i have fixed this properly and all tests are passing.

kunalspathak · 2024-07-16T00:41:17Z

src/coreclr/jit/lsraarm64.cpp

                {
+                    assert(!tgtPrefOp1);
                    srcCount += BuildDelayFreeUses(intrin.op1, intrin.op2, predMask);


btw, I just realized that in #104002 this should have been:

Suggested change

srcCount += BuildDelayFreeUses(intrin.op1, intrin.op2, predMask);

srcCount += BuildDelayFreeUses(intrin.op2, intrin.op1, predMask);

It should actually be fine? intrin.op2 is the RMW node, and BuildDelayFreeUses' second parameter is for the RMW node.

No, if you see the summary docs of BuildDelayFreeUses(), node is the node for which we want to create the RefPosition for, in this case intrin.op2 and rmwNode is the node for which we want to make sure that the register doesn't collide with, in this case, it is intrin.op1. See some examples of places where we call BuildDelayFreeUses().

//------------------------------------------------------------------------ // BuildDelayFreeUses: Build Use RefPositions for an operand that might be contained, // and which may need to be marked delayRegFree // // Arguments: // node - The node of interest // rmwNode - The node that has RMW semantics (if applicable) // candidates - The set of candidates for the uses // useRefPositionRef - If a use RefPosition is created, returns it. If none created, sets it to nullptr. // // REVIEW: useRefPositionRef is not consistently set. Also, sometimes this function creates multiple RefPositions // but can only return one. Does it matter which one gets returned? // // Return Value: // The number of source registers used by the *parent* of this node. // int LinearScan::BuildDelayFreeUses(GenTree* node, GenTree* rmwNode, SingleTypeRegSet candidates, RefPosition** useRefPositionRef)

If I make that change, I get the assertion:

Errors:

Assert failure(PID 15364 [0x00003c04], Thread: 6432 [0x1920]): Assertion failed '!"removeListNode didn't find the node"' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleBinaryOpTest__Sve_CreateMaskForFirstActiveElement_sbyte:RunBasicScenario_UnsafeRead():this' during 'Linear scan register alloc' (IL size 110; hash 0xd48bee8e; FullOpts)

File: C:\work\runtime\src\coreclr\jit\lsrabuild.cpp:69 Image: C:\Users\dotnetperfuser\Desktop\work\windows.arm64.Checked\Tests\Core_Root\corerun.exe

fixed it properly.

kunalspathak

LGTM. Thanks!

Added Sve.CreateBreakPropagateMask

5513cad

dotnet-issue-labeler bot added area-System.Runtime.Intrinsics new-api-needs-documentation labels Jul 11, 2024

dotnet-policy-service bot assigned TIHan Jul 11, 2024

TIHan mentioned this pull request Jul 11, 2024

Arm64: Implement SVE APIs #99957

Closed

TIHan added the arch-arm64 label Jul 11, 2024

Added assert

dd2275c

This was referenced Jul 11, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

TIHan added 3 commits July 11, 2024 10:18

Merge remote-tracking branch 'upstream/main' into sve-create-prop-mask

ac80014

Fixed targetReg and maskReg using the same register

2b8ec02

Minor rename

b88f664

TIHan marked this pull request as ready for review July 11, 2024 19:30

TIHan added 2 commits July 11, 2024 12:31

Formatting

244a08a

No need to use predMask

71b1034

TIHan added 2 commits July 11, 2024 12:51

More formatting

09b3601

Add additional comment

3e9cf63