ARM64-SVE: Add checks for MOVPRFX sequencing (#105514)#106184

a74nh · 2024-08-09T11:18:46Z

As per the Arm Arm, MOVPRFX instructions must be followed by an SVE instruction of specific type, otherwise behaviour is undefined.

Add a function which checks that two instructions in sequence are valid. For now this only cares when the first instruction is a MOVPRFX, but the function could be expanded to check for any pairs, should we need to.

Function is called after code generation in a loop which cycles through all IGs and all instructions.
This ensures the stream is valid after peephole type optimisations.

Alternatively, this function could be called from emitInsSanityCheck(). EMIT_BACKWARDS_NAVIGATION can then be used to get the previous instruction. However, defining this adds an extra variable to instrDesc, reducing the small immediate size from 7bits to 2bits.

Alternatively, this function could be inside every emitIns_ function, allowing emitLastIns to be used for the previous instruction. However, that is a lot of call sites and some could easily be missed. In addition a peephole optimisation could still occur after this point.

Currently requires #106125 for all SVE tests to pass.
AddSequentialAcross tests disabled until #106180 is fixed.

Resolves: #105514

a74nh · 2024-08-09T11:21:14Z

@dotnet/arm64-contrib @kunalspathak

kunalspathak

Suggested some changes. Also, can you also confirm all stress tests are passing?

src/coreclr/jit/emitarm64.cpp

src/coreclr/jit/emit.cpp

kunalspathak · 2024-08-09T13:33:46Z

src/coreclr/jit/emitarm64.cpp

+
+        case IF_SVE_CC_2A: // <Zdn>.<T>, <V><m>
+        case IF_SVE_FU_2A: // <Zda>.<T>, <Zn>.<T>, #<const>
+            assert(secondId->idReg1() != secondId->idReg2());


i was hoping to see lot more validation for registers of firstId as listed in #100743 (comment) or something similar:

i was hoping to see lot more validation for registers of firstId

That should be all at the bottom of the function. I'll double check that I have everything, because the text you have above is a much more verbose version than what I was using.

looks like you have most of the things covered. We need to add validation for:

MOVPRFX cannot be the last instruction in a sequence

The predicated instruction at PC+4 must use the merging predicate. (not sure if this is taken care by the format checks)

MOVPRFX destination register MUST be used as the destination register in the instruction at PC+4, "and is not allowed to be used in any other position other than destructive input"

MOVPRFX cannot be the last instruction in a sequence

Done.

The predicated instruction at PC+4 must use the merging predicate. (not sure if this is taken care by the format checks)

Looking at all the formats, they are all /M or unpredicated. I've added some extra checking to make sure they correspond to the correct movprfx instruction.

MOVPRFX destination register MUST be used as the destination register in the instruction at PC+4, "and is not allowed to be used in any other position other than destructive input"

That's done in the switch. Eg:
assert(secondId->idReg1() != secondId->idReg3());
I had to do it there because you need the format to know what types the registers are.

src/coreclr/jit/emitarm64sve.cpp

Change-Id: Ib78085c631ade5e923cddfda4def7efb362b4587

kunalspathak

Added some more comments on validation.

src/coreclr/jit/emit.cpp

src/coreclr/jit/emitarm64sve.cpp

kunalspathak · 2024-08-12T17:46:41Z

src/coreclr/jit/emitarm64sve.cpp

+        assert(firstId->idReg2() == secondId->idReg2());
+
+        // "and have the same maximum element size (ignoring a fixed 64-bit "wide vector" operand),"
+        assert(firstId->idInsOpt() == secondId->idInsOpt());


can this be true for unpredicated version as well?

The unpredicated movprfx doesn't care about element sizes, it is just MOVPRFX <Zd>, <Zn>.

a74nh · 2024-08-13T09:16:40Z

Remove the changes to GenerateHWIntrinsicTests_Arm.cs.

This PR needs to wait for #106292 (and #106125) until it can be merged

Change-Id: I0a421eacf4895cfb2a30aaa6c483f4a7a3f17dd1

a74nh · 2024-08-14T09:31:00Z

This PR needs to wait for #106292 (and #106125) until it can be merged

Those are merged now. There is nothing blocking this PR.

However - when running the stress testing I see some more errors from this PR. Will investigate.

amanasifkhalid · 2024-08-14T16:33:00Z

However - when running the stress testing I see some more errors from this PR. Will investigate.

@a74nh I'm assuming you're getting a clean run now, right?

a74nh · 2024-08-14T16:39:18Z

Normal testing passes with tiered and untiered. Need to check stress testing

However - when running the stress testing I see some more errors from this PR. Will investigate.

@a74nh I'm assuming you're getting a clean run now, right?

I'm not - but it's not due to this PR.

------------------- {'JitStress': '1'} -------------------
Test failed:
..........................................
..........................................
Sve.AbsoluteDifference<UInt64>(Vector<UInt64>, Vector<UInt64>): ConditionalSelectScenario_TrueValue failed:
    mask: (0, 0)
    left: (8023513538364913332, 10600524560448718829)
   right: (14956876948995427056, 7583671262694082551)
 falseOp: (8023513538364913332, 10600524560448718829)
  result: (0, 0)

I'll raise a bug and take a look at it - I have a reasonable idea of what it might be.

This PR shouldn't need to be blocked on it though.

src/coreclr/jit/emit.cpp

src/coreclr/jit/emitarm64sve.cpp

amanasifkhalid · 2024-08-14T16:41:21Z

src/coreclr/jit/hwintrinsiccodegenarm64.cpp

                HWIntrinsicImmOpHelper helper(this, intrin.op3, node);

-                for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd())
+                for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd(), (targetReg != op1Reg) ? 2 : 1)


What is this syntax doing? It looks like the result of the ternary operator isn't being used.

That change should have been on the line above - the HWIntrinsicImmOpHelper constructor needs to take in the number of instructions that will be generated in each stage of the loop.

amanasifkhalid · 2024-08-14T16:42:31Z

I'll raise a bug and take a look at it - I have a reasonable idea of what it might be.
This PR shouldn't need to be blocked on it though.

Got it, thanks!

a74nh · 2024-08-14T17:35:39Z

I'll raise a bug and take a look at it - I have a reasonable idea of what it might be.

This was due to changes in #106125. I've fixed in this PR because it's related and simple enough - that PR changed the type of movprfx emitted when it shouldn't have.

Also fixed up review comments.

Just checking now that all stress testing passes now.

amanasifkhalid

LGTM if stress tests pass. Thanks!

a74nh · 2024-08-14T18:43:52Z

LGTM if stress tests pass. Thanks!

Full stress testing for all APIs passing now.
ro: https://gist.github.com/a74nh/b690f44a2589873fce3066592d5141fa
and same for r too.

Change request is out-of-date

ARM64-SVE: Add checks for MOVPRFX sequencing

2346feb

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 9, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 9, 2024

a74nh changed the title ~~ARM64-SVE: Add checks for MOVPRFX sequencing~~ ARM64-SVE: Add checks for MOVPRFX sequencing #105514 Aug 9, 2024

a74nh marked this pull request as ready for review August 9, 2024 11:20

a74nh added the arm-sve Work related to arm64 SVE/SVE2 support label Aug 9, 2024

Fix formatting

aa24734

a74nh changed the title ~~ARM64-SVE: Add checks for MOVPRFX sequencing #105514~~ ARM64-SVE: Add checks for MOVPRFX sequencing 105514 Aug 9, 2024

a74nh changed the title ~~ARM64-SVE: Add checks for MOVPRFX sequencing 105514~~ ARM64-SVE: Add checks for MOVPRFX sequencing (105514) Aug 9, 2024

a74nh changed the title ~~ARM64-SVE: Add checks for MOVPRFX sequencing (105514)~~ ARM64-SVE: Add checks for MOVPRFX sequencing (#105514) Aug 9, 2024

kunalspathak suggested changes Aug 9, 2024

View reviewed changes

Move emitInsPairSanityCheck

4c7420a

JulieLeeMSFT assigned a74nh Aug 9, 2024

JulieLeeMSFT added this to the 9.0.0 milestone Aug 9, 2024

kunalspathak reviewed Aug 10, 2024

View reviewed changes

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

a74nh added 3 commits August 12, 2024 11:38

Improve asserts

d33578f

check final instruction

bf3e08e

Merge main

f494959

Change-Id: Ib78085c631ade5e923cddfda4def7efb362b4587

a74nh mentioned this pull request Aug 12, 2024

ARM64-SVE: Ensure MOVPRFX is next to SVE instruction in imm tables #106125

Merged

a74nh added 2 commits August 12, 2024 17:13

Reorder switch table and add movprefxIsPredicated check

2e46d8d

fix formatting

5a517f6

kunalspathak previously requested changes Aug 12, 2024

View reviewed changes

This was referenced Aug 12, 2024

System.IO.Tests crash in CI (Linux arm64) #100441

Closed

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

Closed

a74nh added 3 commits August 13, 2024 08:42

fixups

bcc454f

Remove GenerateHWIntrinsicTests_Arm.cs changes

ebcdebc

move idReg1() check

5752964

build-analysis bot mentioned this pull request Aug 13, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

build-analysis bot mentioned this pull request Aug 13, 2024

msbuild crashes with "MSB0001: Internal MSBuild Error: must be valid" dotnet/dnceng#3304

Closed

3 tasks

Merge main

2585844

Change-Id: I0a421eacf4895cfb2a30aaa6c483f4a7a3f17dd1

a74nh added 4 commits August 14, 2024 12:41

Allow unpredicated movprfx with predicated instruction

52c7ef8

Fix NI_Sve_ExtractVector

29e8efd

Fix for uses with zero masks (eg loads)

8c7e9d3

Fix tied register checking

b9e46ae

JulieLeeMSFT requested review from amanasifkhalid and kunalspathak August 14, 2024 16:06

Merge branch 'main' into movprfxvalidate_github

71637e9

amanasifkhalid reviewed Aug 14, 2024

View reviewed changes

a74nh added 3 commits August 14, 2024 18:31

revert type of movprfx

430c86c

fix comments

8bee58f

Fix use of immediate helpers

4f3610d

amanasifkhalid approved these changes Aug 14, 2024

View reviewed changes

fix formatting

190026b

This was referenced Aug 14, 2024

System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows fails in CI #103586

Open

System.Threading.ThreadPools.Tests.ThreadPoolTests.IOCompletionPortCountConfigVarTest failure - time out, unexpected exit code #106206

Closed

amanasifkhalid merged commit 35a6730 into dotnet:main Aug 14, 2024

a74nh deleted the movprfxvalidate_github branch August 15, 2024 07:37

github-actions bot locked and limited conversation to collaborators Sep 14, 2024

Comments

Conversation

a74nh commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a74nh commented Aug 9, 2024

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a74nh commented Aug 13, 2024

Uh oh!

a74nh commented Aug 14, 2024

Uh oh!

amanasifkhalid commented Aug 14, 2024

Uh oh!

a74nh commented Aug 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid commented Aug 14, 2024

Uh oh!

a74nh commented Aug 14, 2024

Uh oh!

amanasifkhalid left a comment

Choose a reason for hiding this comment

Uh oh!

a74nh commented Aug 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

a74nh commented Aug 9, 2024 •

edited

Loading