Skip to content
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.

Sign up for GitHub

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

Merged
merged 30 commits into from
May 27, 2024
+81 −64
Merged

[RISC-V] Fix invalid operand register in the emitted addition/subtraction code #102074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f2d7eb1
[RISC-V] Added sext_w pseudoinstruction
Bajtazar May 9, 2024
cf7afe4
[RISC-V] Inserted INS_sext_w pseudoinstruction
Bajtazar May 9, 2024
e962811
[RISC-V] Started implementing new overflow logic
Bajtazar May 9, 2024
836f439
[RISC-V] Finished preliminar implementation of bound checks
Bajtazar May 10, 2024
bcdb4a4
[RISC-V] Fixed invalid 32-bit instruction
Bajtazar May 10, 2024
3e3b72f
[RISC-V] Fixed 32-bit addition overflow check assert
Bajtazar May 10, 2024
685f72e
[RISC-V] More fixes in emitter
Bajtazar May 10, 2024
37ed8d2
[RISC-V] Additional fixes
Bajtazar May 10, 2024
71f9c49
[RISC-V] Fixed triple same register problem in emitInsTernary additio…
Bajtazar May 10, 2024
a5d29d8
[RISC-V] Added sext.w to disassembler
Bajtazar May 10, 2024
c6614fa
[RISC-V] Added comments
Bajtazar May 10, 2024
971b36f
[RISC-V] Formatted code
Bajtazar May 10, 2024
4e0264b
[RISC-V] Fixed bug
Bajtazar May 10, 2024
3d0ba8e
[RISC-V] Fixed other bug
Bajtazar May 10, 2024
807acfc
[RISC-V] Fixed bug causing the int32's version to never be emitted
Bajtazar May 10, 2024
4316263
[RISC-V] Fixed assert
Bajtazar May 10, 2024
b841e3c
[RISC-V] Improved comment
Bajtazar May 10, 2024
f82a853
[RISC-V] Fixed comment
Bajtazar May 10, 2024
1f5dfa9
Merge branch 'main' into riscv-fix-overflow-infinite-loop
Bajtazar May 10, 2024
965be5a
[RISC-V] Fixed temp reg acquiring
Bajtazar May 10, 2024
b34f37d
[RISC-V] Removed asserts
Bajtazar May 10, 2024
4371f04
Fixed NodeInternalRegister's GetSingle method's comment
Bajtazar May 10, 2024
ad89ff5
[RISC-V] Revoked more changes
Bajtazar May 14, 2024
a2cdb16
[RISC-V] Revoked more changes
Bajtazar May 14, 2024
a5c582f
[RISC-V] Embedded sext_w into codegen
Bajtazar May 14, 2024
9078647
[RISC-V] Fixed some comments
Bajtazar May 14, 2024
7b24b88
[RISC-V] Added additional comment
Bajtazar May 14, 2024
6a6dc82
[RISC-V] Improvements
Bajtazar May 14, 2024
06dbd79
[RISC-V] Added old comment
Bajtazar May 14, 2024
e464442
Merge branch 'main' into riscv-fix-overflow-infinite-loop
Bajtazar May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegencommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ regNumber NodeInternalRegisters::Extract(GenTree* tree, regMaskTP mask)
}

//------------------------------------------------------------------------
// GetSingleTempReg: There is expected to be exactly one available temporary register
// GetSingle: There is expected to be exactly one available temporary register
// in the given mask in the internal register set. Get that register. No future calls to get
// a temporary register are expected. Removes the register from the set, but only in
// DEBUG to avoid doing unnecessary work in non-DEBUG builds.
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/codegenriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4005,7 +4005,7 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree)
else
{
imm = static_cast<int32_t>(imm);
emit->emitIns_R_R_I(INS_addiw, EA_8BYTE, tmpRegOp1, regOp1, 0);
emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1);
}
regOp1 = tmpRegOp1;
break;
Expand All @@ -4032,7 +4032,7 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree)
}
else
{
emit->emitIns_R_R_I(INS_addiw, EA_8BYTE, tmpRegOp1, regOp1, 0);
emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1);
}
regOp1 = tmpRegOp1;
}
Expand Down Expand Up @@ -5671,13 +5671,13 @@ void CodeGen::genRangeCheck(GenTree* oper)
if (genActualType(length) == TYP_INT)
{
regNumber tempReg = internalRegisters.Extract(oper);
GetEmitter()->emitIns_R_R_I(INS_addiw, EA_4BYTE, tempReg, lengthReg, 0); // sign-extend
GetEmitter()->emitIns_R_R(INS_sext_w, EA_4BYTE, tempReg, lengthReg);
lengthReg = tempReg;
}
if (genActualType(index) == TYP_INT)
{
regNumber tempReg = internalRegisters.GetSingle(oper);
GetEmitter()->emitIns_R_R_I(INS_addiw, EA_4BYTE, tempReg, indexReg, 0); // sign-extend
GetEmitter()->emitIns_R_R(INS_sext_w, EA_4BYTE, tempReg, indexReg);
indexReg = tempReg;
}

Expand Down
134 changes: 75 additions & 59 deletions src/coreclr/jit/emitriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

@clamp03 clamp03 May 10, 2024 •
edited
Loading

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.

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 can add pseudos to the disassm but I would prefer to do it in other PR. Would you mind?

{
assert(isGeneralRegisterOrR0(reg1));
assert(isGeneralRegisterOrR0(reg2));
Expand Down Expand Up @@ -3561,8 +3561,15 @@ void emitter::emitDispInsName(
//}
switch (opcode2)
{
case 0x0: // ADDIW
printf("addiw %s, %s, %d\n", rd, rs1, imm12);
case 0x0: // ADDIW & SEXT.W
if (imm12 == 0)
{
printf("sext.w %s, %s\n", rd, rs1);
}
else
{
printf("addiw %s, %s, %d\n", rd, rs1, imm12);
}
return;
case 0x1: // SLLIW
printf("slliw %s, %s, %d\n", rd, rs1, imm12 & 0x3f); // 6 BITS for SHAMT in RISCV64
Expand Down Expand Up @@ -4980,18 +4987,16 @@ regNumber emitter::emitInsTernary(instruction ins, emitAttr attr, GenTree* dst,
if (dstReg == regOp1)
{
assert(tempReg != regOp1);
assert(REG_RA != regOp1);
saveOperReg1 = tempReg;
saveOperReg2 = regOp2;
emitIns_R_R_I(INS_addi, attr, tempReg, regOp1, 0);
saveOperReg2 = (regOp1 == regOp2) ? tempReg : regOp2;
emitIns_R_R(INS_mov, attr, tempReg, regOp1);
}
else if (dstReg == regOp2)
{
assert(tempReg != regOp2);
assert(REG_RA != regOp2);
saveOperReg1 = regOp1;
saveOperReg2 = tempReg;
emitIns_R_R_I(INS_addi, attr, tempReg, regOp2, 0);
emitIns_R_R(INS_mov, attr, tempReg, regOp2);
}
else
{
Expand All @@ -5002,73 +5007,84 @@ regNumber emitter::emitInsTernary(instruction ins, emitAttr attr, GenTree* dst,

emitIns_R_R_R(ins, attr, dstReg, regOp1, regOp2);

/*
Check if A = B + C
ADD : A = B + C
SUB : B = A - C
In case of addition:
dst = src1 + src2
A = dst
B = src1
C = src2
In case of subtraction:
dst = src1 - src2
src1 = dst + src2
A = src1
B = dst
C = src2
*/
if (needCheckOv)
{
ssize_t imm;
regNumber tempReg1;
regNumber tempReg2;
// ADD : A = B + C
// SUB : C = A - B
bool isAdd = (dst->OperGet() == GT_ADD);
regNumber resultReg = REG_NA;

if (dst->OperGet() == GT_ADD)
{
resultReg = dstReg;
regOp1 = saveOperReg1;
regOp2 = saveOperReg2;
}
else
{
resultReg = saveOperReg1;
regOp1 = dstReg;
regOp2 = saveOperReg2;
Bajtazar marked this conversation as resolved.
Show resolved Hide resolved
}

instruction branchIns = INS_none;
regNumber branchReg1 = REG_NA;
regNumber branchReg2 = REG_NA;

if ((dst->gtFlags & GTF_UNSIGNED) != 0)
{
// if A < B, goto overflow
if (isAdd)
{
tempReg1 = dstReg;
tempReg2 = saveOperReg1;
}
else
{
tempReg1 = saveOperReg1;
tempReg2 = saveOperReg2;
}
codeGen->genJumpToThrowHlpBlk_la(SCK_OVERFLOW, INS_bltu, tempReg1, nullptr, tempReg2);
// if A < B then overflow
branchIns = INS_bltu;
branchReg1 = resultReg;
branchReg2 = regOp1;
}
else
{
tempReg1 = REG_RA;
tempReg2 = codeGen->internalRegisters.Extract(dst);
assert(tempReg1 != tempReg2);
assert(tempReg1 != saveOperReg1);
assert(tempReg2 != saveOperReg2);
regNumber tempReg1 = codeGen->internalRegisters.GetSingle(dst);

ssize_t ui6 = (attr == EA_4BYTE) ? 31 : 63;
emitIns_R_R_I(INS_srli, attr, tempReg1, isAdd ? saveOperReg1 : dstReg, ui6);
emitIns_R_R_I(INS_srli, attr, tempReg2, saveOperReg2, ui6);
branchIns = INS_bne;

emitIns_R_R_R(INS_xor, attr, tempReg1, tempReg1, tempReg2);
if (attr == EA_4BYTE)
{
imm = 1;
emitIns_R_R_I(INS_andi, attr, tempReg1, tempReg1, imm);
emitIns_R_R_I(INS_andi, attr, tempReg2, tempReg2, imm);
}
// if (B > 0 && C < 0) || (B < 0 && C > 0), skip overflow
BasicBlock* tmpLabel = codeGen->genCreateTempLabel();
BasicBlock* tmpLabel2 = codeGen->genCreateTempLabel();
BasicBlock* tmpLabel3 = codeGen->genCreateTempLabel();

emitIns_J_cond_la(INS_bne, tmpLabel, tempReg1, REG_R0);

emitIns_J_cond_la(INS_bne, tmpLabel3, tempReg2, REG_R0);
assert(src1->gtType != TYP_LONG);
assert(src2->gtType != TYP_LONG);

// B > 0 and C > 0, if A < B, goto overflow
emitIns_J_cond_la(INS_bge, tmpLabel, isAdd ? dstReg : saveOperReg1,
isAdd ? saveOperReg1 : saveOperReg2);
emitIns_R_R_R(INS_add, attr, tempReg1, regOp1, regOp2);

codeGen->genDefineTempLabel(tmpLabel2);

codeGen->genJumpToThrowHlpBlk(EJ_jmp, SCK_OVERFLOW);

codeGen->genDefineTempLabel(tmpLabel3);
// if 64-bit addition is not equal to 32-bit addition for 32-bit operands then overflow
branchReg1 = resultReg;
branchReg2 = tempReg1;
}
else
{
assert(attr == EA_8BYTE);
assert(tempReg != tempReg1);
// When the tempReg2 is being used then the tempReg has to be already dead
regNumber tempReg2 = tempReg;

// B < 0 and C < 0, if A > B, goto overflow
emitIns_J_cond_la(INS_blt, tmpLabel2, isAdd ? saveOperReg1 : saveOperReg2,
isAdd ? dstReg : saveOperReg1);
emitIns_R_R_R(INS_slt, attr, tempReg1, resultReg, regOp1);
emitIns_R_R_I(INS_slti, attr, tempReg2, regOp2, 0);

codeGen->genDefineTempLabel(tmpLabel);
// if ((A < B) != (C < 0)) then overflow
Copy link
Member

@clamp03 clamp03 May 10, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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

Copy link
Member

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.

branchReg1 = tempReg1;
branchReg2 = tempReg2;
}
}

codeGen->genJumpToThrowHlpBlk_la(SCK_OVERFLOW, branchIns, branchReg1, nullptr, branchReg2);
}
}
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/instrsriscv64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ INST(nop, "nop", 0, 0x00000013)

//// R_R
INST(mov, "mov", 0, 0x00000013)
INST(sext_w, "sext.w", 0, 0x0000001b)

////R_I
INST(lui, "lui", 0, 0x00000037)
Expand Down
Loading