-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[RISC-V] Lowering multiplication by power of 2 #116783
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
Conversation
* Lower multiplication operations by power of 2 * Add codegen for GT_MUL * Emit slli for GT_MUL with overflow handling
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.
Pull Request Overview
This PR introduces a new lowering optimization for multiplication by power‐of‑2 values by replacing the multiplication with a shift left instruction and adding overflow handling.
- In lowerriscv64.cpp, an early exit is added for power-of‑2 constant operands to mark them as contained.
- In codegenriscv64.cpp, new code generates optimized instructions for multiplication by power of 2, including separate handling for overflow and unsigned cases.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/coreclr/jit/lowerriscv64.cpp | Adds an early exit for contained power-of‑2 multiplication operands before the standard check. |
src/coreclr/jit/codegenriscv64.cpp | Implements code generation for power-of‑2 multiplications with overflow handling and optimizations. |
Comments suppressed due to low confidence (2)
src/coreclr/jit/lowerriscv64.cpp:129
- Consider adding a comment explaining the early return when op2 is a power-of-2 constant to clarify that further containment checks are not needed.
return mul->gtNext;
src/coreclr/jit/codegenriscv64.cpp:1086
- [nitpick] Consider adding a comment to explain why additional slli and srli instructions are emitted for unsigned multiplication to improve clarity for future maintainers.
if (isUnsigned)
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
RISC-V Release-CLR-QEMU: 9083 / 9113 (99.67%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-CLR-VF2: 9083 / 9113 (99.67%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-VF2: 306949 / 308668 (99.44%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-QEMU: 283871 / 284946 (99.62%)
report.xml, report.md, failures.xml, testclr_details.tar.zst Build information and commandsGIT: |
@dotnet-policy-service agree |
CC @dotnet/samsung |
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.
Could use some tests stressing RISC-V corner-cases (if they aren't already covered by existing ones).
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.
Thanks for your review! I'll add tests for any uncovered RISC-V corner cases shortly.
RISC-V Release-CLR-VF2: 9086 / 9116 (99.67%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-CLR-QEMU: 9085 / 9115 (99.67%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-QEMU: 283423 / 284499 (99.62%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-VF2: 303870 / 305621 (99.43%)
report.xml, report.md, failures.xml, testclr_details.tar.zst Build information and commandsGIT: |
@@ -1014,6 +1014,111 @@ void CodeGen::genCodeForIncSaturate(GenTree* tree) | |||
genProduceReg(tree); | |||
} | |||
|
|||
// Generate code for multiplications by power of 2. | |||
void CodeGen::genCodeForMul(GenTreeOp* treeNode) |
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.
Why is this needed instead of it hitting the standard morph optimization which is meant to do the same?
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.
Oops. I think this is good for practicing newcomer.
I missed checking the opt. Sorry, it is my fault.
I close it.
RISC-V Release-CLR-VF2: 9085 / 9115 (99.67%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-CLR-QEMU: 9085 / 9115 (99.67%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-QEMU: 268237 / 269331 (99.59%)
report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-VF2: 494814 / 496559 (99.65%)
report.xml, report.md, failures.xml, testclr_details.tar.zst Build information and commandsGIT: |
Lowering multiplication operations by power of 2, for optimization.
Added codegen for
GT_MUL
, which emitsslli
forGT_MUL
with overflow handling.@SkyShield
@credo-quia-absurdum
part of #84834, cc @dotnet/samsung