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.

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

Conversation

Bajtazar
Copy link
Contributor

@Bajtazar Bajtazar commented May 10, 2024

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, s1
     add     s1, s1, s1
     srli    ra, a0, 63
     srli    a1, s1, 63 ; s1 should be a0 which was the cause of the bug
     xor     ra, ra, a1
     bnez    ra, label_1
     bnez    a1, label_2
     bge     s1, a0, label_1
label_3:
     j       overflow
label_2:
     blt     a0, s1, label_3 
label_1:
     ; valid code

; New long overflow check
     mv      a0, s1
     add     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 a0
     xor     ra, ra, a1
     andi    ra, ra, 1
     andi    a1, a1, 1
     bnez    ra, label_1
     bnez    a1, label_2
     bge     s1, a0, label_1
label_3:
     j       overflow
label_2:
     blt     a0, s1, label_3 
label_1:
    ; valid code

; New int overflow check
     mv      a0, s1
     addw    s1, s1, s1
     add     a1, a0, a0
     bne     s1, a1, overflow
    ; valid code

Part of #84834, cc @dotnet/samsung

@dotnet-issue-labeler dotnet-issue-labeler bot 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-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 10, 2024
Copy link
Contributor

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

@tomeksowi
Copy link
Contributor

; Old long overflow check
     mv      a0, s1
     add     s1, s1, s1
     srli    ra, a0, 63
     srli    a1, s1, 63 ; s1 should be a0 which was the cause of the bug
     xor     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
Copy link
Contributor Author

; Old long overflow check
     mv      a0, s1
     add     s1, s1, s1
     srli    ra, a0, 63
     srli    a1, s1, 63 ; s1 should be a0 which was the cause of the bug
     xor     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

src/coreclr/jit/emitriscv64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/emitriscv64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/emitriscv64.cpp Outdated Show resolved Hide resolved
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label May 10, 2024
@Bajtazar
Copy link
Contributor Author

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
Copy link
Member

clamp03 commented May 22, 2024

@jakobbotsch Could you review this PR? Thank you.

@clamp03 clamp03 requested a review from jakobbotsch May 22, 2024 00:35
@risc-vv
Copy link

risc-vv commented May 24, 2024

RISC-V testing failed on init-build

GIT: e464442

@jakobbotsch
Copy link
Member

/azp run runtime, runtime-coreclr superpmi-diffs

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch jakobbotsch merged commit ca9180b into dotnet:main May 27, 2024
105 of 107 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…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
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants