We legalize the ALU instructions for RISCV according to the hardware mode. W-instructions will be supported in a future patch.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I've added the previous patch legalizing ALU instructions to a child revision of this, since it also attempts to handle w-instructions. Our first major goal is implementing globalisel at -O0; w-instructions are not needed yet.
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 60 | Should we manually edit the formatting here? | |
Comment Actions
As long as you are not interested in vectors, I used this pattern:
getActionDefinitionsBuilder(G_FREEZE)
.legalFor({s8, s16, s32, s64, p0})
.widenScalarToNextPow2(0, /*Min=*/8)
.clampScalar(0, s8, sMaxScalar);clamp to legal range and widen to next power of two to handle weird cases.
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 39 | I don't see any command lines with -mattr=+m, so this branch isn't tested? In fact there are no test cases for mul, udiv, urem, sdiv, or srem. | |
Comment Actions
Add tests for smaller sizes, mul/div/rem variants.
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 39 | You're right, I'll add these. | |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 48 | What is the effect of the clampScalar? You are doing lib call for legal values? | |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 46–47 | I don't think ordering libcall before anything else will work | |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 52 | Is this libcall tested? There is no s128 libcall for RV32, but s64 should use a libcall on RV64. | |
| 76 | G_CONSTANT should only have 1 type. What does .maxScalar(1, XLenLLT); here do? | |
| 79 | How can G_ZEXT/G_SEXT/G_ANYEXT be legal for any types? The source and destination should be different types, and there is only 1 legal type on RISC-V. | |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 79 | The most complete target for Gisel is AArch64. See there legalisation for EXTs: | |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 79 | I think the AArch64 rules are over-permissive and old. It shouldn't be permitting <= register size, only == register size values through | |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 79 | I found remark: unable to legalize instruction: %252:_(<16 x s32>) = G_ZEXT %249:_(<16 x s8>) [-Rpass-missed=gisel-legalize] in the wild. Till today, I have no clue how to legalize it on AArch64. | |
Comment Actions
Breaking the problem into smaller chunks seems helpful.
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 46–47 | I moved the libcall after the clampScalar, but retrospectively I think the original ordering was correct. getActionDefinitionsBuilder({G_MUL, G_SDIV, G_SREM, G_UDIV, G_UREM})
.legalFor({XLenLLT})
.libcallFor({s128})
.clampScalar(0, XLenLLT, XLenLLT);This reads as "These opcodes are legal for XLenLLT. If that check fails, see if the type is s128 and then use a libcall. If that fails, try extending the argument 0 to XLenLLT type." That seems correct to me. | |
| 48 | If I understand correctly, this is being addressed by the conversation with arsenm above. | |
I don't see any command lines with -mattr=+m, so this branch isn't tested?
In fact there are no test cases for mul, udiv, urem, sdiv, or srem.