Skip to content

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented Nov 20, 2023

  • Added placeholder methods for predicate registers. This is taken from Arm64/SVE: Add Uzp1 #94529
  • Added "Z" register display. For now, I have added EA_SCALABLE to differentiate these registers. It can change in future.

Contributes to #94549

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 20, 2023
@ghost ghost assigned kunalspathak Nov 20, 2023
@ghost
Copy link

ghost commented Nov 20, 2023

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

Issue Details
  • Added placeholder methods for predicate registers. This is taken from Arm64/SVE: Add Uzp1 #94529
  • Added "Z" register display. For now, I have added EA_SCALABLE to differentiate these registers. It can change in future.
Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

* Returns an encoding for the specified register used in the 'Pn' position
*/

/*static*/ emitter::code_t emitter::insEncodeReg_Pn(regNumber reg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a shame we can't reuse insEncodeReg_Zn etc - the function is very similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only differentiator is the following portion: Could use macro to share most of that code, but don't think it is worth it (for now).

    assert(emitter::isPredicateRegister(reg));
    emitter::code_t ureg = (emitter::code_t)reg - (emitter::code_t)REG_V0;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not worth sharing. The asserts will be different even if the code is similar. It'll probably either get inlined or comdat folded in a release build anyway.

@kunalspathak kunalspathak marked this pull request as ready for review November 21, 2023 08:38
@kunalspathak
Copy link
Contributor Author

@dotnet/jit-contrib


#if defined(TARGET_XARCH)
#if defined(TARGET_ARM64)
EA_SCALABLE = 0x020,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intention here is to use scalable alongside another size? For example, for an SVE vector of ints:

    theEmitter->emitIns_R_R_R(INS_sve_and, EA_1BYTE|EA_SCALABLE, REG_V0, REG_V1, REG_V2, INS_OPTS_NONE);  // AND     <Zdn>.<T>, <Pg>/M, <Zdn>.<T>, <Zm>.<T>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add the following lane size enum entries in insOpts to represent number of lanes in scalable registers.

INS_OPTS_SCALABLE_B,
INS_OPTS_SCALABLE_H,
INS_OPTS_SCALABLE_S,
INS_OPTS_SCALABLE_D,

Then, for your example, it would be:

   theEmitter->emitIns_R_R_R(INS_sve_and, EA_SCALABLE, REG_V0, REG_V1, REG_V2, INS_OPTS_SCALABLE_B);  // AND     <Zdn>.<T>, <Pg>/M, <Zdn>.<T>, <Zm>.<T>

@BruceForstall
Copy link
Contributor

Were you going to add the predicate registers REGALIAS to this PR or another?

@kunalspathak
Copy link
Contributor Author

Were you going to add the predicate registers REGALIAS to this PR or another?

Added. Can you pleae take another look?

Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kunalspathak kunalspathak merged commit a1af7c3 into dotnet:main Nov 22, 2023
@kunalspathak kunalspathak deleted the predicate-func branch November 22, 2023 05:49
@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Nov 22, 2023
INS_OPTS_SCALABLE_H,
INS_OPTS_SCALABLE_S,
INS_OPTS_SCALABLE_D,

Copy link
Contributor

@a74nh a74nh Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a similar thing in my WIP, but put it inside emitAttr:

#if defined(TARGET_ARM64)
                EA_SIZE_MASK      = 0x01F,
                EA_SCALABLE       = 0x020,
                EA_1BYTE_SCALABLE = EA_1BYTE | EA_SCALABLE,
                EA_2BYTE_SCALABLE = EA_2BYTE | EA_SCALABLE,
                EA_4BYTE_SCALABLE = EA_4BYTE | EA_SCALABLE,
                EA_8BYTE_SCALABLE = EA_8BYTE | EA_SCALABLE,

I'm happy with he way you've done it.

But what do we use for the size?
For a basic AND with integer registers, would you do:

theEmitter->emitIns_R_R_R(INS_sve_and, EA_4BYTE, REG_Z0, REG_P1, REG_Z2, INS_OPTS_SCALABLE_S);

or:

theEmitter->emitIns_R_R_R(INS_sve_and, EA_UNKNOWN, REG_Z0, REG_P1, REG_Z2, INS_OPTS_SCALABLE_S);

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I've just seen EA_SCALABLE. Which gives:

theEmitter->emitIns_R_R_R(INS_sve_and, EA_SCALABLE, REG_Z0, REG_P1, REG_Z2, INS_OPTS_SCALABLE_S);

@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants