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
[RISC-V] Fix invalid operand register in the emitted addition/subtraction code #102074
base: main
Are you sure you want to change the base?
Conversation
…n and subtraction logic
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
If s1 would be a0 in this snippet, both ra and a1 would have the same value after right shifts, so the xor below would always return 0, no? |
Yes, this second shift was supposed to calculate whether the original second operand was negative but since the logic responsible for preserving the original's operands value wasn't prepared for the case where both of the operand registers were same and thus allowing the bug to happen. After fixing it it also implied that |
@@ -588,7 +588,7 @@ void emitter::emitIns_R_R( | |||
{ | |||
code_t code = emitInsCode(ins); | |||
|
|||
if (INS_mov == ins) | |||
if (INS_mov == ins || INS_sext_w == ins) |
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.
Have you discussed in your team to use pseudo code actively in JIT?
Actually, I want to remove all pseudo codes like mov
in JITC because some pseudo codes (contains multiple normal instructions) cannot be used in JIT.
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.
IMO simple "renaming" pseudos like sext.w, bnez, mov, etc, are fine. We're already using them, they convey the intention a bit more clearly, and they make the assembly look closer to print-outs from native compilers.
I agree about multi-instruction pseudos, though.
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.
Okay. Actually, we used few pesudos in few code locations, however AFAIK it is not printed properly in dump. I just let those instruction because I hope removing later. If you want to use pseudos. Could you add such instructions like 'mov' and 'nop' in emitDispInsName
too?
Thank you.
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 can add pseudos to the disassm but I would prefer to do it in other PR. Would you mind?
Which tests can you fix by this PR? |
|
||
codeGen->genDefineTempLabel(tmpLabel); | ||
// if ((A < B) != (C < 0)) then overflow |
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.
For subtraction, is it correct comment?
I think if B = A - C
is right for sub, this comment should be update or if A = B - C
is right for sub, then // if B > A then overflow
in line 5037 should be update.
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.
IMO it is Ok, I've used the property of the overflows in RISCV64 that operations ignore overflow and wraps around 2^64. Due to this if A = B - C overflows then B = A + C does too. I've shuffled registers at the beginning according to the type of the operation so that at the end I just checks whether there is an addition overflow
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 was confused. Thank you for your explanation.
It fixes |
@jakobbotsch Could you review this PR? Thank you. |
RISC-V testing failed on init-buildGIT: e464442 |
Fixes invalid operand register in the emitted addition/subtraction code when both of the operand registers are same. Slightly improves quality of the generated code. Also introduces
sext.w
preudoinstruction to replace double-shift sign extension snippetsExamples of old and new code:
Part of #84834, cc @dotnet/samsung