[RISC-V] Fix invalid operand register in the emitted addition/subtraction code by Bajtazar · Pull Request #102074 · dotnet/runtime

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar · 2024-05-10T12:21:46Z

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 snippets

Examples of old and new code:

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New long overflow check mv a0, s1add s1, s1, s1 slt a1, s1, a0 slti a0, a0,0 bne a1, a0, overflow ; valid code; Old int overflow check mv a0, s1 addw s1, s1, s1 srli ra, a0,31 srli a1, s1,31 ; same problem with s1 instead of a0xor ra, ra, a1 andi ra, ra,1 andi a1, a1,1 bnez ra, label_1 bnez a1, label_2 bge s1, a0, label_1label_3: j overflowlabel_2: blt a0, s1, label_3 label_1: ; valid code; New int overflow check mv a0, s1 addw s1, s1, s1add a1, a0, a0 bne s1, a1, overflow ; valid code

Part of #84834, cc @dotnet/samsung

…n and subtraction logic

dotnet-policy-service · 2024-05-10T12:22:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

tomeksowi · 2024-05-10T12:32:32Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1

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?

Bajtazar · 2024-05-10T12:43:38Z

; Old long overflow check mv a0, s1add s1, s1, s1 srli ra, a0,63 srli a1, s1,63 ; s1 should be a0 which was the cause of the bugxor ra, ra, a1
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 ra would always be equal to zero rendering it and its branch useless in this case, so I've decided to reshape the emitter a little bit

Bajtazar · 2024-05-14T14:39:11Z

Which tests can you fix by this PR?

It fixes JIT/jit64/rtchecks/overflow/overflow04_add/overflow04_add.sh and during testing it also seems to fix JIT/opt/virtualstubdispatch/bigvtbl/bigvtbl_cs_d/bigvtbl_cs_d.sh

clamp03 · 2024-05-22T00:35:04Z

@jakobbotsch Could you review this PR? Thank you.

risc-vv · 2024-05-24T13:38:06Z

RISC-V testing failed on init-build

GIT: e464442

jakobbotsch · 2024-05-27T09:27:35Z

/azp run runtime, runtime-coreclr superpmi-diffs

azure-pipelines · 2024-05-27T09:28:07Z

Azure Pipelines successfully started running 2 pipeline(s).

…tion code (dotnet#102074) * [RISC-V] Added sext_w pseudoinstruction * [RISC-V] Inserted INS_sext_w pseudoinstruction * [RISC-V] Started implementing new overflow logic * [RISC-V] Finished preliminar implementation of bound checks * [RISC-V] Fixed invalid 32-bit instruction * [RISC-V] Fixed 32-bit addition overflow check assert * [RISC-V] More fixes in emitter * [RISC-V] Additional fixes * [RISC-V] Fixed triple same register problem in emitInsTernary addition and subtraction logic * [RISC-V] Added sext.w to disassembler * [RISC-V] Added comments * [RISC-V] Formatted code * [RISC-V] Fixed bug * [RISC-V] Fixed other bug * [RISC-V] Fixed bug causing the int32's version to never be emitted * [RISC-V] Fixed assert * [RISC-V] Improved comment * [RISC-V] Fixed comment * [RISC-V] Fixed temp reg acquiring * [RISC-V] Removed asserts * Fixed NodeInternalRegister's GetSingle method's comment * [RISC-V] Revoked more changes * [RISC-V] Revoked more changes * [RISC-V] Embedded sext_w into codegen * [RISC-V] Fixed some comments * [RISC-V] Added additional comment * [RISC-V] Improvements * [RISC-V] Added old comment

Bajtazar added 14 commits May 9, 2024 14:42

[RISC-V] Added sext_w pseudoinstruction
f2d7eb1

[RISC-V] Inserted INS_sext_w pseudoinstruction
cf7afe4

[RISC-V] Started implementing new overflow logic
e962811

[RISC-V] Finished preliminar implementation of bound checks
836f439

[RISC-V] Fixed invalid 32-bit instruction
bcdb4a4

[RISC-V] Fixed 32-bit addition overflow check assert
3e3b72f

[RISC-V] More fixes in emitter
685f72e

[RISC-V] Additional fixes
37ed8d2

[RISC-V] Fixed triple same register problem in emitInsTernary additio…
71f9c49
…n and subtraction logic

[RISC-V] Added sext.w to disassembler
a5d29d8

[RISC-V] Added comments
c6614fa

[RISC-V] Formatted code
971b36f

[RISC-V] Fixed bug
4e0264b

[RISC-V] Fixed other bug
3d0ba8e

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 10, 2024

dotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 10, 2024

Bajtazar added 3 commits May 10, 2024 14:49

[RISC-V] Fixed bug causing the int32's version to never be emitted
807acfc

[RISC-V] Fixed assert
4316263

[RISC-V] Improved comment
b841e3c

tomeksowi suggested changes May 10, 2024
View reviewed changes

Comment threadsrc/coreclr/jit/emitriscv64.cpp Outdated
Comment threadsrc/coreclr/jit/emitriscv64.cpp Outdated

tomeksowi suggested changes May 10, 2024
View reviewed changes

Comment threadsrc/coreclr/jit/emitriscv64.cpp Outdated

[RISC-V] Fixed comment
f82a853

clamp03 assigned Bajtazar May 10, 2024

clamp03 added the arch-riscv Related to the RISC-V architecture label May 10, 2024

Bajtazar added 3 commits May 10, 2024 15:35

Merge branch 'main' into riscv-fix-overflow-infinite-loop
1f5dfa9

[RISC-V] Fixed temp reg acquiring
965be5a

[RISC-V] Removed asserts
b34f37d

clamp03 reviewed May 10, 2024
View reviewed changes

Comment threadsrc/coreclr/jit/emitriscv64.cpp

build-analysisBot mentioned this pull request May 10, 2024
Test failure in System.Numerics.Tensors.Tests.SingleGenericTensorPrimitives.SpanDestinationFunctions_SpecialValues #101731
Closed

tomeksowi reviewed May 11, 2024
View reviewed changes

Comment threadsrc/coreclr/jit/emitriscv64.cpp Outdated

Bajtazar added 7 commits May 14, 2024 10:09

[RISC-V] Revoked more changes
ad89ff5

[RISC-V] Revoked more changes
a2cdb16

[RISC-V] Embedded sext_w into codegen
a5c582f

[RISC-V] Fixed some comments
9078647

[RISC-V] Added additional comment
7b24b88

[RISC-V] Improvements
6a6dc82

[RISC-V] Added old comment
06dbd79

Merge branch 'main' into riscv-fix-overflow-infinite-loop
e464442

build-analysisBot mentioned this pull request May 15, 2024
NativeAOT legs timing out in CI #102239
Closed

Bajtazar mentioned this pull request May 15, 2024
[RISC-V] Add pseudoinstructions to disassembler #102260
Merged

Bajtazar marked this pull request as ready for review May 21, 2024 08:08

Bajtazar requested review from clamp03 and tomeksowi May 21, 2024 08:09

tomeksowi approved these changes May 21, 2024
View reviewed changes

clamp03 approved these changes May 22, 2024
View reviewed changes

clamp03 requested a review from jakobbotsch May 22, 2024 00:35

sirntar approved these changes May 22, 2024
View reviewed changes

jakobbotsch approved these changes May 27, 2024
View reviewed changes

jakobbotsch mentioned this pull request May 27, 2024
System.Management.Tests fails with "CONTRACT VIOLATION by LoaderAllocator::SetHandleValue" #102719
Closed

jakobbotsch merged commit ca9180b into dotnet:mainMay 27, 2024

github-actionsBot locked and limited conversation to collaborators Jun 27, 2024