-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Update the bit field of encoding function names #96082
Conversation
@dotnet/arm64-contrib @a74nh @SwapnilGaikwad |
LGTM! |
src/coreclr/jit/emitarm64.cpp
Outdated
*/ | ||
|
||
/*static*/ emitter::code_t emitter::insEncodeReg_R_18_to_17(regNumber reg) | ||
/*static*/ emitter::code_t emitter::insEncodeReg_R_17_to_16(regNumber reg) | ||
{ | ||
assert(isIntegerRegister(reg)); | ||
emitter::code_t ureg = (emitter::code_t)reg; | ||
assert((ureg >= 0) && (ureg <= 32)); |
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.
assert((ureg >= 0) && (ureg <= 32)); | |
assert((ureg >= 0) && (ureg <= 3)); |
?
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.
psel
uses this encoding and I am not sure what register represents by 2 bits. The manual says "Is the 32-bit name of the vector select register W12-W15, encoded in the "Rv" field.". @a74nh do you know?
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.
PSEL <Pd>, <Pn>, <Pm>.<T>[<Wv>, <imm>]
It's the Wv
in the above. That's quite an odd instruction encoding - it's trying to fit in 4 registers, a 5bit immediate and the element size.
It's not explicitly mentioned, but w12
would be 00
, w13
would be 01
, etc
So, the assert should be (ureg >= 12) && (ureg <= 15)
.
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.
hoping to get it address in #95996 to skip a CI run.
The arm manual parsing tool had a bug where I was not taking into account the extra space, resulting in function names off by 1. Renamed the function names.