-
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
JIT: ARM64 SVE format encodings, SVE_IH_3A
to SVE_JO_3A
#95994
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsNote: I still have a few more formats to do as well as some cleanup. Contributes to #94549 Adds the following SVE formats:
|
SVE_IH_3A
SVE_IH_3A
to SVE_JO_3A
SVE_IH_3A
to SVE_JO_3A
SVE_IH_3A
to SVE_JO_3A
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.
Sorry to take a while to review this.
inline static bool insOptsScalableWordsOrQuadwords(insOpts opt) | ||
{ | ||
// `opt` is any of the standard word, quadword and above scalable types. | ||
return (insOptsScalableWords(opt) || (opt == INS_OPTS_SCALABLE_Q)); |
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.
Just a check for INS_OPTS_SCALABLE_Q
seems little odd. Probably #96692 would revise this.
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.
That PR would revise this. Should I wait for the PR to be merged then adjust this?
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.
you are adding just 1 entry and since your PR was out for a while now, I would suggest to merge it after you address the feedback.
// Therefore, by default return NONE due to ambiguity. | ||
case IF_SVE_AH_3A: | ||
case IF_SVE_DB_3A: | ||
// TODO: Handle these cases. |
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.
Need to handle this?
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.
We haven't implemented those formats yet, and also I'm not sure how we will represent <Pg>/<ZM>
.
|
||
if (canEncodeSveElemsize_dtype(ins)) | ||
{ | ||
code = insEncodeSveElemsize_dtype(ins, optGetSveElemsize(id->idInsOpt()), code); |
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.
src/coreclr/jit/emitarm64.cpp
Outdated
const bool notLastRegister = (i != listSize - 1); | ||
emitDispSveReg(currReg, opt, notLastRegister); | ||
currReg = (currReg == REG_V31) ? REG_V0 : REG_NEXT(currReg); | ||
if ((listSize == 2) || (((unsigned)currReg + listSize - 1) > (unsigned)REG_V31)) |
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.
(listSize == 2)
Can't this be true for any listSize > 1
?
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.
can you add a unit test that passes REG_V31
and see if they are disassembled correctly?
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.
Sure
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 already have unit tests that use REG_V31. It prints out ld4b { z31.b, z0.b, z1.b, z2.b }, p2/z, [x1, #-0x20, mul vl]
, but I may not have one when listsize is 1.
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.
(listSize == 2)
Can't this be true for any
listSize > 1
?
looks like this is not yet addressed.
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.
Will look at it, I guess we may not need that check.
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.
We do need the check, but I slightly changed it.
case IF_SVE_JE_3A: | ||
case IF_SVE_JO_3A: | ||
// This does not have to be printed as hex. | ||
// We only do it because the capstone disassembly displays this immediate as hex. |
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.
does this work properly with LATEDISASM? Regardless, I would remove the comment about capstone.
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 will check
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.
coredistools has trouble decoding some instructions here compared to capstone, so it's going to be different
@kunalspathak this is ready again. |
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.
Looks good, just need to address one outstanding feedback.
src/coreclr/jit/emitarm64.cpp
Outdated
const bool notLastRegister = (i != listSize - 1); | ||
emitDispSveReg(currReg, opt, notLastRegister); | ||
currReg = (currReg == REG_V31) ? REG_V0 : REG_NEXT(currReg); | ||
if ((listSize == 2) || (((unsigned)currReg + listSize - 1) > (unsigned)REG_V31)) |
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.
(listSize == 2)
Can't this be true for any
listSize > 1
?
looks like this is not yet addressed.
@kunalspathak Ok, addressed the remaining feedback. |
Is this true for all the instructions? Currently only I would rewrite it to something like: assert(listSize > 0);
// We do not want the short-hand for list size of 1 or 2.
if ((listSize <= 2) || (((unsigned)currReg + listSize - 1) > (unsigned)REG_V31))
{
for (unsigned i = 0; i < listSize; i++)
{
const bool notLastRegister = (i != listSize - 1);
emitDispSveReg(currReg, opt, notLastRegister);
currReg = (currReg == REG_V31) ? REG_V0 : REG_NEXT(currReg);
}
}
else
{
// short-hand. example: { z0.s - z2.s } which is the same as { z0.s, z1.s, z2.s }
emitDispSveReg(currReg, opt, false);
printf(" - ");
emitDispSveReg((regNumber)(currReg + listSize - 1), opt, false);
} |
Anything with the syntax |
@kunalspathak I put in your change. |
Right, but if you see runtime/src/coreclr/jit/emitarm64.cpp Line 17433 in 904c439
listSize == 2 and with the code in the PR, it will use the short-hand.
I think we should separate the 2 methods, |
It won't use the short-hand since the list size is 2. |
@kunalspathak I renamed |
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
Thanks! |
Contributes to #94549
Adds the following SVE formats:
Capstone is in red, Jit is in green