This patch legalizes ALU operations according to operations supported natively by RISC-V. This includes necessary lowering for sign extend and zero extend operations on smaller types, allowing single word operations where supported on RV64, and split operations on double XLen types where appropriate. Libcalls are utilized where hardware does not support an operation and/or type (specifically for M-extension operations).
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/CodeGen/RISCV/GlobalISel/legalizer/alu32.mir | ||
|---|---|---|
| 10 | I didn't realise this, though I'm considering leaving it for consistency with tests in other backends? Also I am going to look into using the update_mir_test_checks tool instead. | |
Comment Actions
Expand patch to correctly cover more methods of types being legalized - IE: single word instructions on RV64, split operations (add with carry etc.) and libcalls up to s128.
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 105 | Currently RISC-V defines signext_inreg as legal when we know that the inner type is i32. However with GlobalISel there is no way to retrieve the inner type to define legality, as the action of G_SEXT_INREG is determined by an immediate operand. I was currently in the process of rewriting these patches to properly handle single word operations on RV64, of which ADDW, SUBW and MULW rely on pattern matching an i32 to i64 signext_inreg of the corresponding operation. Though since we cannot allow the G_SEXT_INREG to survive correctly I feel the only option for now may be to directly select these instructions at the legalization stage. | |
Comment Actions
Add testing for shifts. Include a version of handling single word operations on RV64 involving directly lowering to the final opcode.
| llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp | ||
|---|---|---|
| 797 ↗ | (On Diff #400000) | Can you split the mul libcall handling into a separate patch? |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
| 115 | You can do auto NewOp0 = MIRBuilder.buildAnyExt(s64, MI.getOperand(1).getReg()) | |
| 150 | You're not really legalizing anything here, you're just directly selecting which I would advise against | |
| llvm/test/CodeGen/RISCV/GlobalISel/legalizer/alu32.mir | ||
| 10 | No, just delete it. Nearly every IR section can be deleted | |
Comment Actions
Address review comments; primarily avoid selecting directly until the instruction selector.
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | You shouldn't have this function, you're just repeating totally ordinary promotion the legalizer will handle by default | |
| 105 | The immediate and the inner type are equivalent representations of the same thing. You can and should define the legality rules the same. | |
| 105 | Ditto, you're repeating basic work the legalizer does by default | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | It doesn't do this by default though. By default the destination gets widened without a G_SEXT or a G_ZEXT, so any pattern we could look to select later on ends up disappearing. I can use the widenScalarSrc as a helper but the widenScalarDst employed by the default lowering will give the wrong behaviour. | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | It does if you set your legalization rules to promote these operations. The new result type is the requested promoted type, with a truncate to the original register. The SextInReg here is unnecessary, and also doesn't really do anything since you're truncating right back to the original result type, discarding the extended bits. | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | Sorry I'm still not getting it. How do I set legalization rules to promote these operations? The only similar options I see are minScalar/clampScalar which will destroy any pattern we could hope to select? Might you be able to explain more? | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | Remove your customFor. I'm not sure what you mean by "destroy any pattern we could hope to select" - the extensions and the operation are independently legalized and selected operations. If you would like to see G_SEXT_INREG in your selection patterns (something you cannot rely on), you need to mark G_SEXT_INREG legal and the artifact combiner will produce it for you | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | I would need some pattern indicating these operations were originally 32 bits to be detectable, because we could then select the appropriate instructions for doing 32 bit operations on 64 bit registers. If I remove the custom legalization here I lose that information because the operation just appears as a 64 bit operation. I could make this work by doing some custom selection code to check if only the lower 32 bits of the result are used. The other alternative is to make these operations legal for 32 bits for these operations (which isn't how the SelectionDAG approaches this issue) and then select the 32 bit operations. | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | So you have a 32-bit add that uses 64-bit registers, and ignores the high bits of the sources and extends the result? This sounds like how AMDGPU handles 16-bit operations in 32-bit registers. We pass through the 16-bit ops to the selector | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | Precisely. I have been trying that as an alternative implementation but without having 32-bit subregisters I don't see how I can create type-legal patterns to select operations operating on 32 bits and create operations which take 32 bit registers. I would do something like `Pat<(i64 (anyext (i32 (OpNode (i32 (trunc GPR:$rs1)), (i32 (trunc GPR:$rs2)))))), (OPW GPR:$rs1, GPR:$rs2)>;, but I definitely cannot guarantee that the anyext/truncates will remain, so I could end up being unable to match the pattern. Without subregisters I can't just select the i32 version of the operation since I'd be replacing it with an i64 version and breaking the type constraints of the instruction selector. For the SelectionDAG code we take the approach that the only legal type is i64 on RV64, then select the 'half-register' variants based on analysing the used bits. | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | The types added to the register class can be smaller than the full register size. AMDGPU adds 16-bit types to 32-bit registers. The selection process just happens to pick 32-bit register classes in the end. | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | If you don't want to change the DAG path to work this way, I would still change what you're doing here to work more like what the DAG is doing with the known bits during the selection | |
| llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 86–89 | Thanks, I think this will be my preferred approach. The instruction selection will have to have custom code to find out about known bits. | |
Comment Actions
LGTM with some nits. I also still think this isn't the best strategy for handling 32-bit ops for 64 but changing that should also change the DAG path
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 31 ↗ | (On Diff #511198) | Don't use reference to LLT |
| 49 ↗ | (On Diff #511198) | Braces |
| 105 ↗ | (On Diff #511198) | I believe this is pre-set for you |
| 123 ↗ | (On Diff #511198) | I believe this is pre-set for you |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 106 ↗ | (On Diff #524396) | Using AnyExt for operands of promoted SDIV/SREM is incorrect from the semantics of the Generic MachineIR. That puts garbage in the upper elements. @arsenm do you agree? |
| 122 ↗ | (On Diff #524396) | This also seems wrong for the semantics of the generic IR. G_SHL needs operand 1 zero extend. Operand 0 can be zero extend. @arsenm do you agree? |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 122 ↗ | (On Diff #524396) | Add 1 to all of the operand numbers I wrote. I didn't account for defs. |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 122 ↗ | (On Diff #524396) | I think you meant any extend for op 0 of G_SHL, but yes |
| llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp | ||
|---|---|---|
| 122 ↗ | (On Diff #524396) | Oops yes. I did mean any extend. |
clang-format: please reformat the code
- getActionDefinitionsBuilder(G_AND) - .legalFor({s64}) - .clampScalar(0, s64, s64); + getActionDefinitionsBuilder(G_AND).legalFor({s64}).clampScalar(0, s64, s64);