This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][GlobalISel] Legalize types for ALU operations
AbandonedPublic

Authored by nitinjohnraj on Mar 18 2020, 4:35 AM.

Details

Summary

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).

Diff Detail

Event Timeline

lewis-revill created this revision.Mar 18 2020, 4:35 AM

Fix copy/paste mistakes in check lines

lewis-revill edited the summary of this revision. (Show Details)

Add XLenLLT, take into account M extension.

Joe added a comment.Mar 20 2020, 7:29 AM

What about G_OR and G_XOR? Is there a reason why they're not listed?

Support AND/OR/XOR.

Joe added a comment.Apr 8 2020, 6:04 AM

LGTM, but probably wait for someone more experienced to upvote :)

arsenm added a subscriber: arsenm.Apr 8 2020, 7:00 AM
arsenm added inline comments.
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
52

returning false here is entirely pointless, you can just delete this

llvm/test/CodeGen/RISCV/GlobalISel/legalizer/alu32.mir
10

You don't need the IR section here

Removed unnecessary line

lewis-revill marked an inline comment as done.Apr 10 2020, 1:14 AM
lewis-revill added inline comments.
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.

Use utils/update_mir_test_checks.py

lewis-revill edited the summary of this revision. (Show Details)Apr 12 2020, 11:52 AM

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.

lewis-revill added inline comments.Apr 16 2020, 9:15 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
106

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.

Add testing for shifts. Include a version of handling single word operations on RV64 involving directly lowering to the final opcode.

Tweak test order

Add target flags to calls in tests.

lenary resigned from this revision.Jan 14 2021, 10:10 AM
rkruppe removed a subscriber: rkruppe.Jan 14 2021, 10:19 AM
arsenm added inline comments.Jan 17 2022, 4:40 PM
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
130

You can do auto NewOp0 = MIRBuilder.buildAnyExt(s64, MI.getOperand(1).getReg())

165

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

Address review comments; primarily avoid selecting directly until the instruction selector.

lewis-revill retitled this revision from [WIP][RISCV][GlobalISel] Legalize types for ALU operations to [RISCV][GlobalISel] Legalize types for ALU operations.Feb 9 2022, 3:14 AM
arsenm added inline comments.Feb 9 2022, 8:43 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

You shouldn't have this function, you're just repeating totally ordinary promotion the legalizer will handle by default

106

The immediate and the inner type are equivalent representations of the same thing. You can and should define the legality rules the same.

120

Ditto, you're repeating basic work the legalizer does by default

lewis-revill added inline comments.Feb 16 2022, 5:54 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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.

arsenm added inline comments.Feb 16 2022, 8:04 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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.

lewis-revill added inline comments.Mar 14 2022, 9:03 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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?

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 9:03 AM
arsenm added inline comments.Mar 14 2022, 9:33 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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

lewis-revill added inline comments.Mar 15 2022, 3:59 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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.

arsenm added inline comments.Mar 15 2022, 3:08 PM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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

lewis-revill added inline comments.Mar 16 2022, 10:06 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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.

arsenm added inline comments.Mar 16 2022, 4:56 PM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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.

arsenm added inline comments.Mar 16 2022, 5:03 PM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

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

lewis-revill added inline comments.Mar 29 2022, 9:10 AM
llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp
101–104

Thanks, I think this will be my preferred approach. The instruction selection will have to have custom code to find out about known bits.

nitinjohnraj commandeered this revision.Apr 5 2023, 1:29 PM
nitinjohnraj added a reviewer: lewis-revill.
arsenm accepted this revision.Apr 8 2023, 5:15 AM

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

This revision is now accepted and ready to land.Apr 8 2023, 5:15 AM
nitinjohnraj marked 4 inline comments as done.

Addressed nits

nitinjohnraj requested review of this revision.May 22 2023, 11:03 AM

Since the approval is a bit old, could we reconfirm that this patch is good to go?

arsenm added inline comments.May 22 2023, 1:30 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
60 ↗(On Diff #524396)

Braces

75–76 ↗(On Diff #524396)

I think this is a really ugly way to wrap legalize rules and clang-format is wrong

arsenm accepted this revision.May 22 2023, 1:41 PM
This revision is now accepted and ready to land.May 22 2023, 1:41 PM
craig.topper added inline comments.May 22 2023, 1:50 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
147 ↗(On Diff #524396)

Why is ASHR legalized with a ZExtInReg?

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.h
21 ↗(On Diff #524396)

alphabetize

craig.topper added inline comments.May 22 2023, 1:56 PM
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.
G_ASHR needs operand 0 sign extended and operand 0 zero extended
G_LSHR, G_UDIV and G_UREM needs both operands zero extended.

@arsenm do you agree?

craig.topper requested changes to this revision.May 22 2023, 1:57 PM
This revision now requires changes to proceed.May 22 2023, 1:57 PM
craig.topper added inline comments.May 22 2023, 2:04 PM
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.

arsenm added inline comments.May 22 2023, 2:13 PM
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

craig.topper added inline comments.May 22 2023, 2:49 PM
llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
122 ↗(On Diff #524396)

Oops yes. I did mean any extend.

evandro removed a subscriber: evandro.May 22 2023, 5:24 PM
nitinjohnraj planned changes to this revision.Jun 12 2023, 10:45 AM
nitinjohnraj marked 2 inline comments as done.

We do not plan on handling w-instructions for globalisel at -O0. For legalization without w-instructions, see D152726.

llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
75–76 ↗(On Diff #524396)

I completely agree. Should I manually format this part?

nitinjohnraj abandoned this revision.Aug 22 2023, 2:57 AM

Done in other patches