Arm64/Sve: Predicated Abs, Predicated/UnPredicated Add, Conditional Select #100743

kunalspathak · 2024-04-07T09:45:54Z

Based on the feedback from @tannergooding in #100134 (comment), I reworked #100134 a little bit and also included the ConditionalSelect I implemented in #100718.

The change in design is that we do not touch the HWIntrinsic node until lowerer. In lowerer, we wrap the node in ConditionalSelect(mask, original_operation, falseVal) and contain the original operation. This lets us easily determine which API should map to the predicate vs. unpredicated version of the instruction.

For APIs like Abs which only has predicated version, in lowerer, we wrap it in ConditionalSelect(ptrueAll, Abs, zero). During containment analysis, for ConditionalSelect, we check if 2nd operand is scalable SVE instrinsic and if yes, we mark it as contained. During codegen, the ConditionalSelect that has op2 as contained needs to go through the "predicated" version of the instruction.
For APIs like Add that has both predicated and unpredicated versions, they are handled differently. For unpredicated Add, it does not get marked as contained and in codegen, generates unpredicated version. In order to generate predicated version of add, user has to write a code such that Add is wrap inside a conditional, e.g. ConditionalSelect(mask, Add(x,y), b). If yes, then it follows same path as Abs and generates predicate version of the instruction.

There are still some handling needs to be done for RMW, but want to get feedback on the design before I move forward. The sample test case along with the output can be found in ~~https://gist.github.com/kunalspathak/bc4e917ced68bef793d11fcbd050162c~~
https://gist.github.com/kunalspathak/1fb0b17f0908ba26e46f0cd146ab05b8 .

~~I still need to handle cases where user writes ConditionalSelect(mask, Abs(x), y). Currently we end of expanding Abs with another ConditionalSelect.~~

Thanks @tannergooding for the design discussion and helping me understand the concepts.

Change-Id: Ie8cfe828595da9a87adbc0857c0c44c0ce12f5b2

This reverts commit e9fa735.

Still some handling around RMW is needed, but this basically works

dotnet-issue-labeler · 2024-04-07T09:46:01Z

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.

src/coreclr/jit/lowerarmarch.cpp

kunalspathak · 2024-04-24T07:08:58Z

@tannergooding - can you take another look? I have added bunch of scenarios to use `ConditionalSelect() on unary/binary operations including:

Pass op1 as falseValue in ConditionalSelect() done on top of the API
Pass op2 as falseValue in ConditionalSelect() done on top of the API
Pass Zero as falseValue in ConditionalSelect() done on top of the API
Pass Zero as maskValue in ConditionalSelect() done on top of the API
Pass AllBitSet as maskValue in ConditionalSelect() done on top of the API
Make sure maskValue has mix of 0s as well.

I also tested the newly added test cases using https://github.com/a74nh/runtime/blob/api_github/sve_api/stress_tester.py and they all pass.

tannergooding · 2024-04-24T17:26:28Z

src/coreclr/jit/hwintrinsic.cpp

    {
        assert(numArgs > 0);
        GenTree* op1 = retNode->AsHWIntrinsic()->Op(1);
+        if (intrinsic == NI_Sve_ConditionalSelect)


Not important for this PR, but this is potentially something that should be handled in gtNewSimdCndSelNode instead and then more generally as part of morph to capture values that don't materialize as constants until later.

will do it in follow-up PR.

a74nh · 2024-04-25T14:11:44Z

I'm getting a failure with the abs and add tests:

❯ $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_Abs_int
14:07:31.270 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Abs_int()
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True


Assert failure(PID 1134821 [0x001150e5], Thread: 1134821 [0x1150e5]): Assertion failed 'isVectorRegister(reg2)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleUnaryOpTest__Sve_Abs_int:RunBasicScenario_UnsafeRead():this' during 'Generate code' (IL size 83; hash 0x0487a6e9; Tier0)

    File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/emitarm64sve.cpp:2248
    Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

[1]    1134821 abort (core dumped)  $CORE_ROOT/corerun  Sve_Abs_int

Backtrace:

#0  DBG_DebugBreak () at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/pal/src/arch/arm64/debugbreak.S:7
#1  0x0000ffffed1029d0 in DebugBreak () at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/pal/src/debug/debug.cpp:407
#2  0x0000ffffecdf8440 in assertAbort (why=0xffffecc8c644 "isVectorRegister(reg2)",
    file=0xffffecc66283 "/home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/emitarm64sve.cpp", line=2248)
    at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/error.cpp:288
#3  0x0000ffffed099178 in emitter::emitInsSve_R_R (this=0xaaaaab99a400, ins=INS_sve_movprfx, attr=EA_SCALABLE, reg1=REG_V16, reg2=REG_COUNT,
    opt=INS_OPTS_NONE, sopt=INS_SCALABLE_OPTS_NONE) at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/emitarm64sve.cpp:2248
#4  0x0000ffffed0e8bf0 in CodeGen::genHWIntrinsic (this=0xaaaaab999b08, node=<optimized out>)
    at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:444
#5  0x0000ffffecdc3440 in CodeGen::genCodeForBBlist (this=0xaaaaab999b08)
    at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/codegenlinear.cpp:472
#6  0x0000ffffecdb6aa0 in CodeGen::genGenerateMachineCode (this=0xaaaaab999b08)
    at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/codegencommon.cpp:1877
#7  0x0000ffffecdc1a70 in CodeGenPhase::DoPhase (this=<optimized out>)
    at /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/codegen.h:1703

Looks like reg2 is invalid in frame 3. reg2=REG_COUNT

a74nh · 2024-04-25T14:13:13Z

Also some (but not all) of the conditional tests are failing:

❯ $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_ConditionalSelect_sbyte
14:12:14.806 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConditionalSelect_sbyte()
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario_UnsafeRead
Sve.ConditionalSelect<SByte>(Vector<SByte>, Vector<SByte>, Vector<SByte>): RunBasicScenario_UnsafeRead failed:
 firstOp: (29, 69, 117, 63, 117, 109, 74, 55, 16, 108, 36, 47, 71, 38, 19, 120)
secondOp: (56, 113, 53, 30, 67, 23, 97, 124, 31, 20, 23, 120, 105, 44, 50, 103)
 thirdOp: (44, 17, 40, 89, 16, 50, 66, 38, 4, 12, 56, 112, 105, 17, 76, 72)
  result: (-1, -1, 0, 0, 67, 23, 97, 124, 31, 20, 23, 120, 105, 44, 50, 103)

Beginning scenario: RunBasicScenario_Load
Sve.ConditionalSelect<SByte>(Vector<SByte>, Vector<SByte>, Vector<SByte>): RunBasicScenario_Load failed:
 firstOp: (29, 69, 117, 63, 117, 109, 74, 55, 16, 108, 36, 47, 71, 38, 19, 120)
secondOp: (56, 113, 53, 30, 67, 23, 97, 124, 31, 20, 23, 120, 105, 44, 50, 103)
 thirdOp: (44, 17, 40, 89, 16, 50, 66, 38, 4, 12, 56, 112, 105, 17, 76, 72)
  result: (-1, -1, 0, 0, 67, 23, 97, 124, 31, 20, 23, 120, 105, 44, 50, 103)

Beginning scenario: RunReflectionScenario_UnsafeRead
Beginning scenario: RunLclVarScenario_UnsafeRead
Beginning scenario: RunClassFldScenario
Beginning scenario: RunStructLclFldScenario
Beginning scenario: RunStructFldScenario
System.Exception: One or more scenarios did not complete as expected.
   at JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConditionalSelect_sbyte() in /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/obj/linux.arm64.Checked/Managed/JIT/HardwareIntrinsics/Arm/Sve/Sve_ro/Sve_ro/gen/Sve.ConditionalSelect.sbyte.cs:line 62
   at Program.<<Main>$>g__TestExecutor2723|0_2724(StreamWriter tempLogSw, StreamWriter statsCsvSw, <>c__DisplayClass0_0&) in /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/obj/linux.arm64.Checked/Managed/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/generated/XUnitWrapperGenerator/XUnitWrapperGenerator.XUnitWrapperGenerator/FullRunner.g.cs:line 68199
14:12:15.143 Failed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConditionalSelect_sbyte()

kunalspathak · 2024-04-25T15:22:31Z

Thanks @a74nh - i will take a look.

kunalspathak · 2024-04-25T22:30:24Z

@a74nh - The Abs failures are known and are fixed in #101515. With that PR and main, all the tests are passing for me as seen in https://gist.github.com/kunalspathak/73e0cfd469a70fd4b5466bada45df2eb. So perhaps, it could be 256-bit machine difference? Can you try restricting it to 128-bit and check if they still fail for you?

a74nh · 2024-04-26T14:54:27Z

So perhaps, it could be 256-bit machine difference? Can you try restricting it to 128-bit and check if they still fail for you?

Yes, it looks like the conditionalselect failures are 256bit issues. Works on 256bit machine when I restrict the vector length to 128bit.

…elect (dotnet#100743) * JIT ARM64-SVE: Add Sve.Abs() and Sve.Add() Change-Id: Ie8cfe828595da9a87adbc0857c0c44c0ce12f5b2 * Fix sve scaling in enitIns_R_S/S_R * Revert "Fix sve scaling in enitIns_R_S/S_R" This reverts commit e9fa735. * Fix sve scaling in enitIns_R_S/S_R * Restore testing * Use NaturalScale_helper for vector load/stores * wip * Add ConditionalSelect() APIs * Handle ConditionalSelect in JIT * Add test coverage * Update the test cases * jit format * fix merge conflicts * Make predicated/unpredicated work with ConditionalSelect Still some handling around RMW is needed, but this basically works * Misc. changes * jit format * jit format * Handle all the conditions correctly * jit format * fix some spacing * Removed the assert * fix the largest vector size to 64 to fix dotnet#100366 * review feedback * wip * Add SVE feature detection for Windows * fix the check for invalid alignment * Revert "Add SVE feature detection for Windows" This reverts commit ed7c781. * Handle case where Abs() is wrapped in another conditionalSelect * jit format * fix the size comparison * HW_Flag_MaskedPredicatedOnlyOperation * Revert the change in emitarm64.cpp around INS_sve_ldr_mask/INS_sve_str_mask * Fix the condition for lowering * address review feedback for movprfx * Move the special handling of Vector<>.Zero from lowerer to importer * Rename IsEmbeddedMaskedOperation/IsOptionalEmbeddedMaskedOperation * Add more test coverage for conditionalSelect * Rename test method name * Add more test coverage for conditionalSelect:Abs * jit format * Add logging on test methods * Add the missing movprfx for abs * Add few more scenarios where falseVal is zero * Make sure LoadVector is marked as explicit needing mask * revisit the codegen logic * Remove commented code and add some other comments * jit format --------- Co-authored-by: Alan Hayward <alan.hayward@arm.com>

a74nh and others added 21 commits March 22, 2024 10:46

JIT ARM64-SVE: Add Sve.Abs() and Sve.Add()

0d437e3

Change-Id: Ie8cfe828595da9a87adbc0857c0c44c0ce12f5b2

Fix sve scaling in enitIns_R_S/S_R

e9fa735

Revert "Fix sve scaling in enitIns_R_S/S_R"

5acc122

This reverts commit e9fa735.

Fix sve scaling in enitIns_R_S/S_R

15e893d

Restore testing

c22f458

Use NaturalScale_helper for vector load/stores

508a52a

Merge remote-tracking branch 'origin/main' into api_abs_github

5825c20

Merge remote-tracking branch 'origin/main' into api_abs_github

9081fa3

wip

d4f3c86

Add ConditionalSelect() APIs

20defbd

Handle ConditionalSelect in JIT

c70367f

Add test coverage

1896b21

Update the test cases

2e0d022

jit format

185fcfa

Merge branch 'sve-conditional-select' into abi_abs_conditional

88e8f87

fix merge conflicts

ebcb174

Make predicated/unpredicated work with ConditionalSelect

28aa7b7

Still some handling around RMW is needed, but this basically works

Misc. changes

1c7b4b2

jit format

6adf6c6

jit format

ecf4e01

Merge remote-tracking branch 'origin/main' into abi_abs_conditional

c6fef82

dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 7, 2024

dotnet-policy-service bot assigned kunalspathak Apr 7, 2024

build-analysis bot mentioned this pull request Apr 7, 2024

[browser][MT] TIMED_OUT but 0 was expected #99888

Closed

a74nh reviewed Apr 8, 2024

View reviewed changes

src/coreclr/jit/lowerarmarch.cpp Outdated Show resolved Hide resolved

kunalspathak added 2 commits April 8, 2024 17:11

Handle all the conditions correctly

d780156

jit format

eb4d1dc

build-analysis bot mentioned this pull request Apr 9, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

kunalspathak added 2 commits April 24, 2024 00:03

Remove commented code and add some other comments

c78e0c7

jit format

6aa2386

This was referenced Apr 24, 2024

clang crashing on osx #99150

Closed

WasmTestOnChrome-System.Runtime.Tests.WorkItemExecution fails with exit code 3221226505 #101464

Closed

tannergooding reviewed Apr 24, 2024

View reviewed changes

tannergooding approved these changes Apr 24, 2024

View reviewed changes

kunalspathak mentioned this pull request Apr 24, 2024

System.Text.Json.Tests timeout in Linux-Debug-x64-Mono_Interpreter_Debug-Ubuntu.2204.Amd64.Open #101508

Closed

kunalspathak merged commit dd24e9c into dotnet:main Apr 24, 2024
163 of 171 checks passed

kunalspathak deleted the abi_abs_conditional branch April 24, 2024 18:05

kunalspathak mentioned this pull request Apr 24, 2024

Arm64/Sve: Implement ConditionalSelect API #100718

Closed

kunalspathak restored the abi_abs_conditional branch April 24, 2024 19:20

This was referenced Apr 24, 2024

JIT ARM64-SVE: Add Sve.LoadVector*ZeroExtendTo*() #101291

Merged

Arm64: Add SVE/SVE2 support in .NET 9 #93095

Closed

Handle case where falseValue is contained #101515

Merged

JIT ARM64-SVE: Add Sve.Abs() and Sve.Add() #100134

Closed

This was referenced Apr 25, 2024

Long Running Test: System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers #101522

Closed

Dead lettering tests #101524

Closed

[WASI][AOT] LLVM ERROR: out of memory #101533

Closed

build-analysis bot mentioned this pull request Apr 25, 2024

Timeout in System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Cookies_Http3.GetAsync_SetCookieContainerAndCookieHeader_BothCookiesSent #101553

Closed

build-analysis bot mentioned this pull request May 2, 2024

[wasm][AOT] emcc : error - received SIGKILL (-9) #89402

Open

kunalspathak mentioned this pull request May 9, 2024

Arm64/Sve: Implement SVE Math *Multiply* APIs #102007

Merged

github-actions bot locked and limited conversation to collaborators May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arm64/Sve: Predicated Abs, Predicated/UnPredicated Add, Conditional Select #100743

Arm64/Sve: Predicated Abs, Predicated/UnPredicated Add, Conditional Select #100743

kunalspathak commented Apr 7, 2024 •

edited

Loading

dotnet-issue-labeler bot commented Apr 7, 2024

kunalspathak commented Apr 24, 2024

tannergooding Apr 24, 2024

kunalspathak Apr 24, 2024

a74nh commented Apr 25, 2024

a74nh commented Apr 25, 2024

kunalspathak commented Apr 25, 2024

kunalspathak commented Apr 25, 2024

a74nh commented Apr 26, 2024 •

edited

Loading

Arm64/Sve: Predicated Abs, Predicated/UnPredicated Add, Conditional Select #100743

Arm64/Sve: Predicated Abs, Predicated/UnPredicated Add, Conditional Select #100743

Conversation

kunalspathak commented Apr 7, 2024 • edited Loading

dotnet-issue-labeler bot commented Apr 7, 2024

kunalspathak commented Apr 24, 2024

tannergooding Apr 24, 2024

Choose a reason for hiding this comment

kunalspathak Apr 24, 2024

Choose a reason for hiding this comment

a74nh commented Apr 25, 2024

a74nh commented Apr 25, 2024

kunalspathak commented Apr 25, 2024

kunalspathak commented Apr 25, 2024

a74nh commented Apr 26, 2024 • edited Loading

kunalspathak commented Apr 7, 2024 •

edited

Loading

a74nh commented Apr 26, 2024 •

edited

Loading