-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add Predicate register placeholder methods and Z registers #95016
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Details
|
| * Returns an encoding for the specified register used in the 'Pn' position | ||
| */ | ||
|
|
||
| /*static*/ emitter::code_t emitter::insEncodeReg_Pn(regNumber reg) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;There was a problem hiding this comment.
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.
|
@dotnet/jit-contrib |
|
|
||
| #if defined(TARGET_XARCH) | ||
| #if defined(TARGET_ARM64) | ||
| EA_SCALABLE = 0x020, |
There was a problem hiding this comment.
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>There was a problem hiding this comment.
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>|
Were you going to add the predicate registers |
Added. Can you pleae take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| INS_OPTS_SCALABLE_H, | ||
| INS_OPTS_SCALABLE_S, | ||
| INS_OPTS_SCALABLE_D, | ||
|
|
There was a problem hiding this comment.
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);
?
There was a problem hiding this comment.
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);
EA_SCALABLEto differentiate these registers. It can change in future.Contributes to #94549