Move arm64 insOpts entries into insScalableOpts by a74nh · Pull Request #96692 · dotnet/runtime

a74nh · 2024-01-09T16:30:47Z

insOpts has a max size of 6 bits, and it is getting full.

For some of the options, they are only required to specify the encoding group, after this only the lane size (_S etc) is needed. Move these to a new enum.

ghost · 2024-01-09T16:31:00Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

insOpts has a max size of 6 bits, and it is getting full.

For some of the options, they are only required to specify the encoding group, after this only the lane size (_S etc) is needed. Move these to a new enum insGroupOpts.

Author:	a74nh
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

a74nh · 2024-01-09T16:33:20Z

This moves all the entries that can be moved. All the remaining insOpts values have additional use later.

INS_OPTS_SCALABLE_x_WITH_PREDICATE_MERGE - these are required for the type of the predicate.
INS_OPTS_SCALABLE_x_WITH_SIMD_VECTOR - these are required for the special printing of the vector type.

a74nh · 2024-01-09T16:33:34Z

@kunalspathak @BruceForstall

insOpts has a max size of 6 bits, and it is getting full. For some of the options, they are only required to specify the encoding group, after this only the lane size (_S etc) is needed. Move these to a new enum insGroupOpts.

kunalspathak · 2024-01-09T18:23:50Z

@dotnet/arm64-contrib

BruceForstall

LGTM. I'll let someone more involved in coding these comment.

kunalspathak · 2024-01-09T19:38:17Z

This moves all the entries that can be moved

I was hoping to move all the INS_OPTS_SCALABLE* entries in a separate enum group and just have 1 entry INS_OPTS_SCALABLE in the existing insOpts. When required, can we not compose the entries of insGroupOpts based on ins/format, etc.?

a74nh · 2024-01-10T11:10:56Z

I've managed to remove the _WITH_SIMD_VECTOR entries as well. These just require the destination vector to be printed differently, and that can be determined from the standard insOpts.

I'll take a closer look at the rest.

a74nh · 2024-01-10T12:12:21Z

The only remaining additional insOpt left is INS_OPTS_SCALABLE_x_WITH_PREDICATE_MERGE.

Why was this needed?
MOVPRFX <Zd>.<T>, <Pg>/<ZM>, <Zn>.<T>

This has a single encoding group (IF_SVE_AH_3A).
The predicate register can either be /z or /m. This is encoded as bit M (position 16) in the instruction.
Eg:

movprfx z13.b, p0/z, z31.b
movprfx z31.d, p7/m, z22.d

This option needs encoding somewhere in the instrDesc. My thoughts are:

Use insOpts in the way already being used.
Predicate registers are only 0-15, but are stored with space for 0-31. Repurpose the unused high bit. I suspect this breaks things and is fiddly to keep correct.
Repurpose _idReg3Scaled, it's a single bit that is not required for these instructions. I've given this a go in the latest update.

If the above is all ok, then there are some additional clean ups I can do with all the assert checks.

a74nh · 2024-01-10T12:24:34Z

I was hoping to move all the INS_OPTS_SCALABLE* entries in a separate enum group and just have 1 entry INS_OPTS_SCALABLE in the existing insOpts.

I'm thinking at minimum we still need the 4 entires (_B, _H, _S, _D). The size of the lane needs storing somewhere. It's not available in emitAttr (as that is just EA_SCALABLE). insOpts seems the best place as it already has entires for neon (8B, 4H etc).

kunalspathak

If the above is all ok, then there are some additional clean ups I can do with all the assert checks.

This looks fine to me.

kunalspathak · 2024-01-10T14:47:03Z

 // IF_SVE_AJ_3A
 #ifdef ALL_ARM64_EMITTER_UNIT_TESTS_SVE_UNSUPPORTED
-    theEmitter->emitIns_R_R_R(INS_sve_addqv, EA_8BYTE, REG_V21, REG_V7, REG_P22, INS_OPTS_SCALABLE_B_WITH_SIMD_VECTOR);
+    theEmitter->emitIns_R_R_R(INS_sve_addqv, EA_8BYTE, REG_V21, REG_P7, REG_V22, INS_OPTS_SCALABLE_B);


looks like there was a typo? This reminds me, that we should double check these instructions encoding with LATE_DISASM and mark them as supported wherever applicable.

Yes, this was a typo.
Problem with adding them is that capstone exits when it doesn't recognise an instruction, so it breaks the diffs.

a74nh · 2024-01-10T15:23:24Z

Thoughts about this:

INCP has two variants - one to a Z register, one to general register:
INCP <Zdn>.<T>, <Pm>.<T>
INCP <Xdn>, <Pm>.<T>

The first is encoded as:
theEmitter->emitIns_R_R(INS_sve_incp, EA_SCALABLE, REG_V3, REG_P3, INS_OPTS_SCALABLE_H);

The second currently requires a scalableOpts:
theEmitter->emitIns_R_R(INS_sve_incp, EA_8BYTE, REG_R7, REG_P7, INS_OPTS_SCALABLE_D, INS_SCALABLE_OPTS_WITH_SCALAR);

But, we could remove the scalableOpts and internally check that the destination register is R (and assert the size is EA_8BYTE).
theEmitter->emitIns_R_R(INS_sve_incp, EA_8BYTE, REG_R7, REG_P7, INS_OPTS_SCALABLE_D);

a74nh · 2024-01-10T16:57:03Z

But, we could remove the scalableOpts and internally check that the destination register is R (and assert the size is EA_8BYTE). theEmitter->emitIns_R_R(INS_sve_incp, EA_8BYTE, REG_R7, REG_P7, INS_OPTS_SCALABLE_D);

I decided to do this as it simplifies the code and makes it less ambiguous.

a74nh · 2024-01-10T17:32:26Z

That's everything cleaned up.

I've settled on insScalableOpts entries are only used when an instruction has multiple variants, and the variants can't be determined by register names alone.

For example, addv uses a simd scalar register as the destination, but it doesn't need to use INS_SCALABLE_OPTS_WITH_SIMD_SCALAR because there is only one version of addv.

kunalspathak

LGTM. Added some questions before we merge.

kunalspathak · 2024-01-11T06:44:42Z

                           // (predicated)
        case IF_SVE_CR_3A: // ........xx...... ...gggnnnnnddddd -- SVE extract element to SIMD&FP scalar register
            elemsize = id->idOpSize();
-            assert(insOptsScalableWithSimdScalar(id->idInsOpt())); // xx


Hhm, so turns out, we won't be able to do the validation in emitInsSanityCheck()

No, we've lost the information by this point.

What would be useful here would be if we had REG_Z1 which is distinct from REG_V1. This way we could assert isSveVectorRegister().

AIUI, when predicate registers are added, REGNUM_BITS needs increasing from 6 bits to 7 bits. That would allow for 32 general + 32 neon + 16 SVE + 32 predicate.

There would then have to be a step somewhere around register allocations to convert SVE register numbers to/from NEON register numbers to ensure there is really only one set.

Alternatively, we could still have a distinct REG_Z1, but inside emitIns_R_R_R() it converts to REG_V1 before storing to _idReg1. That means you can assert in emitIns_R_R_R() but not in emitInsSanityCheck().

Either would allow for the removal of INS_SCALABLE_OPTS_WITH_SIMD_SCALAR too.

kunalspathak · 2024-01-11T06:47:17Z

            assert(isVectorRegister(reg3));
-            assert(insOptsScalableWithSimdVector(opt));
+            assert(insOptsScalable(opt));
+            assert(sopt == INS_SCALABLE_OPTS_NONE);


shouldn't this be INS_SCALABLE_OPTS_WITH_SIMD_SCALAR?

likewise for few below where previously we had assert(insOptsScalableWithSimdVector(opt))

There is only a single encoding for andqv. It always has a destination of a SIMD vector. This is the same for every instruction that uses a SIMD vector. In all these cases, the only use of setting sopt would be for an assert check. Removing the setting of sopt meant there were no uses of _WITH_SIMD_VECTOR and it could be removed.

Meanwhile, have a look above at andv. This always has a destination of a SIMD scalar. This is the same for some other instructions (eg saddv), but there are some instructions which can optionally have a destination of SVE Vector or SIMD scalar (eg clasta or mov). We have a choice here:

Always pass _WITH_SIMD_SCALAR to any instruction with a SIMD scalar.

Only pass _WITH_SIMD_SCALAR to instructions where there is an option (clasta/mov). For instructions without a option (andv) do not use an sopt.

The PR goes with the second option. Why?....

Finally, have a look at movprfx. This can have optionally have zero or merge predicate, and uses _PREDICATE_MERGE to distinguish. If we were to insist on always passing _WITH_SIMD_SCALAR to andv, then technically we'd have to pass _PREDICATE_MERGE to every instruction that uses a predicate merge. Which is a lot of instructions, and becomes a pain to ensure we're always using it.

That makes sense. I am wondering if we can capture this in a comment somewhere so the other developers can follow and reason about this design.

I added this comment for the insScalableOpts definition in instr.h

kunalspathak · 2024-01-11T14:43:06Z

+            (opt == INS_OPTS_SCALABLE_D) || (opt == INS_OPTS_SCALABLE_Q));
+}
+
+inline static bool insOptsScalableStandard(insOpts opt)


wondering why we need a different method for INS_OPTS_SCALABLE_Q?

Most instructions don't allow Q sized registers. It's only a small few.

I think insOptsScalable() should still check for everything, even though that's not the default.

Prior to this PR, most instructions were using insOptsScalableSimple() but that vanished when I got rid of all the enum entries. For simplicity, I could rename insOptsScalableStandard() to insOptsScalableSimple() and that'd reduce the diff in this PR.

I am thinking why not just have insOptsScalableStandard() (or insOptsScalableSimple()). Just have 1 method which is insOptsScalable() that takes care of _Q as well.

That will mean for most instructions we will allow Q to be passed as a size.

Which would then probably assert when trying to encode the instruction, in insEncodeElemsize() (because it doesn't support Q)

If an instruction wrongly passes _Q where it is not supposed to, insEncodeElemsize() will correctly assert, right? I am trying to understand why _Q needs special treatment than others.

Only reason it would need special treatment is that we already have the various size checks for other instructions. insOptsScalableWide() is used for widening instructions (B,H,S only), insOptsScalableFloat() is used for float types (H,S,D), insOptsScalableWords() is just S and D, etc etc. Those are used throughout emitInsSanityCheck(), instead of using insOptsScalable(). It would seem odd to check for restricted element sizes for some instructions, and then for others allow Q (via use of insOptsScalable()).

I see, alright that makes sense then.

nit for next PR: we can rewrite it by:

inline static bool insOptsScalable(insOpts opt) { // `opt` is any of the scalable types. return (insOptsScalableStandard(opt) || (opt == INS_OPTS_SCALABLE_Q)); }

kunalspathak · 2024-01-11T19:26:05Z

/azp run runtime-coreclr superpmi-diffs

azure-pipelines · 2024-01-11T19:26:19Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak

LGTM

* Move arm64 insOpts entries into insGroupOpts insOpts has a max size of 6 bits, and it is getting full. For some of the options, they are only required to specify the encoding group, after this only the lane size (_S etc) is needed. Move these to a new enum insGroupOpts. * Nits * Rename to insScalableOpts * Remove _WITH_SIMD_VECTOR * Remove _WITH_PREDICATE_MERGE. Reuse _idReg3Scaled. * Remove insOptsScalableSimple * Better insScalableOpts entry names * Add INS_SCALABLE_OPTS_NONE asserts * Remove INS_SCALABLE_OPTS_WITH_SCALAR * insScalableOpts descriptions * Remove uses of INS_SCALABLE_OPTS_WITH_SIMD_SCALAR for single variants * Remove unused sopt from emitIns_R_R * Add insScalableOptsNone * Restore unreached() for capstone unsupported * Add insOptsScalableStandard

ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jan 9, 2024

a74nh mentioned this pull request Jan 9, 2024

JIT ARM64: Add IF_SVE_DT_3A, IF_SVE_DU_3A, IF_SVE_DX_3A, IF_SVE_DY_3A #96201

Merged

Move arm64 insOpts entries into insGroupOpts

92c3032

insOpts has a max size of 6 bits, and it is getting full. For some of the options, they are only required to specify the encoding group, after this only the lane size (_S etc) is needed. Move these to a new enum insGroupOpts.

a74nh force-pushed the insGroupOpts_github branch from db72ceb to 92c3032 Compare January 9, 2024 16:34

kunalspathak mentioned this pull request Jan 9, 2024

JIT: ARM64 SVE format encodings, SVE_IH_3A to SVE_JO_3A #95994

Merged

21 tasks

BruceForstall approved these changes Jan 9, 2024

View reviewed changes

Comment thread src/coreclr/jit/emitarm64.h Outdated

Comment thread src/coreclr/jit/emitarm64.cpp Outdated

kunalspathak reviewed Jan 9, 2024

View reviewed changes

Comment thread src/coreclr/jit/instr.h Outdated

a74nh added 4 commits January 10, 2024 09:55

Nits

edf011a

Rename to insScalableOpts

e69cc8b

Merge branch main

39729ba

Remove _WITH_SIMD_VECTOR

380bf10

Remove _WITH_PREDICATE_MERGE. Reuse _idReg3Scaled.

b1f6a51

kunalspathak reviewed Jan 10, 2024

View reviewed changes

Remove insOptsScalableSimple

c3ea0a3

a74nh added 3 commits January 10, 2024 15:27

Better insScalableOpts entry names

6b7632c

Add INS_SCALABLE_OPTS_NONE asserts

1cfff00

Remove INS_SCALABLE_OPTS_WITH_SCALAR

478ac67

a74nh added 2 commits January 10, 2024 17:11

insScalableOpts descriptions

5d05d6d

Remove uses of INS_SCALABLE_OPTS_WITH_SIMD_SCALAR for single variants

13a7783

Remove unused sopt from emitIns_R_R

82a3bac

a74nh changed the title ~~Move arm64 insOpts entries into insGroupOpts~~ Move arm64 insOpts entries into insScalableOpts Jan 10, 2024

a74nh mentioned this pull request Jan 10, 2024

Add Arm64 encodings for SVE IF_SVE_CX_4A_A to IF_SVE_HT_4A group #96214

Merged

kunalspathak reviewed Jan 11, 2024

View reviewed changes

kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Jan 11, 2024

a74nh added 4 commits January 11, 2024 12:08

Add insScalableOptsNone

d751331

Restore unreached() for capstone unsupported

4ae0b5e

Merge branch main

2129459

Add insOptsScalableStandard

e15373c

kunalspathak reviewed Jan 11, 2024

View reviewed changes

kunalspathak approved these changes Jan 11, 2024

View reviewed changes

kunalspathak merged commit 1ad2ac0 into dotnet:main Jan 11, 2024

a74nh deleted the insGroupOpts_github branch January 12, 2024 10:00

github-actions Bot locked and limited conversation to collaborators Feb 12, 2024

Conversation

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

Uh oh!

Uh oh!

ghost commented Jan 9, 2024

Uh oh!

a74nh commented Jan 9, 2024

Uh oh!

a74nh commented Jan 9, 2024

Uh oh!

kunalspathak commented Jan 9, 2024

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kunalspathak commented Jan 9, 2024

Uh oh!

Uh oh!

a74nh commented Jan 10, 2024

Uh oh!

a74nh commented Jan 10, 2024

Uh oh!

a74nh commented Jan 10, 2024

Uh oh!

kunalspathak left a comment

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!

Uh oh!

a74nh commented Jan 10, 2024

Uh oh!

a74nh commented Jan 10, 2024

Uh oh!

a74nh commented Jan 10, 2024

Uh oh!

kunalspathak left a comment

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!

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!

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!

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!

kunalspathak commented Jan 11, 2024

Uh oh!

azure-pipelines Bot commented Jan 11, 2024

Uh oh!

a74nh commented Jan 9, 2024 •

edited

Loading