This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][GlobalISel] Select ALU GPR instructions
ClosedPublic

Authored by craig.topper on Mar 19 2020, 12:22 PM.

Details

Summary

Some instruction selection patterns required for ALU GPR instructions have already been automatically imported from existing TableGen descriptions - this patch simply adds testing for them. Logic for selecting constants and copies has been added, along with the first of the GIComplexPatternEquiv definitions required to select the shiftMaskXLen ComplexPattern.

Diff Detail

Event Timeline

lewis-revill created this revision.Mar 19 2020, 12:22 PM

Add tests for AND/OR/XOR.

Use utils/update_mir_test_checks.py

lewis-revill retitled this revision from [RISCV][GlobalISel] Add tests for selecting ALU GPR instructions to [WIP][RISCV][GlobalISel] Select ALU GPR instructions.
lewis-revill edited the summary of this revision. (Show Details)

Add custom selection for copies and for constants. Significantly expand tests over more types.

Bug fix - ensure selectConstant produces copies with fully constrained registers.

arsenm added a subscriber: arsenm.Aug 11 2020, 12:36 PM

Missing copy tests for all permutations of virtual and physical registers

llvm/lib/Target/RISCV/RISCVInstructionSelector.cpp
114–115 ↗(On Diff #258538)

This can't early return, it still needs to constrain a virtual source

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

Rebased. Added the first GIComplexPatternEquiv definition in order to handle the operands to shifts, which are now covered by a ComplexPattern in SelectionDAG. Updated the tests to match the output of the regbank selector.

lewis-revill planned changes to this revision.Jan 14 2022, 7:52 AM

Still need to address testing permutations of copying virtual/physical registers.

lenary removed a subscriber: lenary.Jan 14 2022, 7:53 AM
rkruppe removed a subscriber: rkruppe.Jan 14 2022, 8:56 AM
arsenm added inline comments.Jan 17 2022, 4:02 PM
llvm/lib/Target/RISCV/RISCVInstructionSelector.cpp
97–108 ↗(On Diff #400005)

This function name is misleading. Can you restructure to use something more like getRegClassForTypeOnBank (which AArch64 has a FIXME to make a generic TargetRegisterInfo method)

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

Restructure slightly, add more cases for selectCopy along with tests, add selection patterns for RV64 W instructions.

lewis-revill marked 2 inline comments as done.Feb 1 2022, 7:23 AM
lewis-revill retitled this revision from [WIP][RISCV][GlobalISel] Select ALU GPR instructions to [RISCV][GlobalISel] Select ALU GPR instructions.Feb 9 2022, 3:14 AM
arsenm added inline comments.Feb 9 2022, 8:31 AM
llvm/lib/Target/RISCV/RISCVGISel.td
30 ↗(On Diff #404931)

Should sext_inreg be legal?

llvm/lib/Target/RISCV/RISCVInstructionSelector.cpp
46 ↗(On Diff #404931)

What is this parameter?

109 ↗(On Diff #404931)

SrcReg.isPhysical()

120 ↗(On Diff #404931)

Why not just print the whole instruction?

145 ↗(On Diff #404931)

Should not construct one off MachineIRBuilders. The selector usually just directly uses BuildMI

171 ↗(On Diff #404931)

Didn't erase the instruction

lewis-revill added inline comments.Apr 7 2022, 5:31 AM
llvm/lib/Target/RISCV/RISCVGISel.td
30 ↗(On Diff #404931)

This is what I'm stuck on right now. It really should to match the SelectionDAG code, at least for i32 -> i64. However there is no way to map G_SEXT_INREG to sext_inreg because sext_inreg uses a value type as a DAG operand, whereas G_SEXT_INREG uses a constant which indicates the size of the original type. So no patterns defined which use sext_inreg will work for GlobalISel. I think my only option left is to do custom selection code.

Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 5:31 AM
arsenm added inline comments.Apr 7 2022, 6:21 AM
llvm/lib/Target/RISCV/RISCVGISel.td
30 ↗(On Diff #404931)

You can either do custom selection code, or you could add support for sext_inreg in GlobalISelEmitter. It's a special case so it requires special support

craig.topper commandeered this revision.Aug 20 2023, 11:11 AM
craig.topper added a reviewer: lewis-revill.
craig.topper updated this revision to Diff 551865.EditedAug 20 2023, 12:12 PM

Rebase. Still needs work.

Having s32 as a valid type in RV64 isn't working because GlobalISelEmitter doesn't really understand HwModes.

craig.topper planned changes to this revision.Aug 20 2023, 12:12 PM
evandro removed a subscriber: evandro.Aug 21 2023, 12:28 PM

Rebase aftering fixing GlobalISel tablegen to handle HwMode.

Add test cases for addi, andi, ori, and xori.

Still need to work on slli(w), srli(w), srai(w), and addiw.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 28 2023, 10:11 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
craig.topper reopened this revision.Aug 28 2023, 10:15 AM
craig.topper edited the summary of this revision. (Show Details)Sep 2 2023, 9:40 PM

Add tests for shifting by immediate. Still need fixes for i32 shifts on RV64.

Add custom selection for ADDIW, SRAIW, SRLIW, and SLLIW.

Support G_SUB with immediate explicitly. It is not canonicalized to G_ADD.

Ok I think this is ready for review now. All the immediate cases are tested and working

nitinjohnraj added inline comments.Sep 15 2023, 10:22 AM
llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
104 ↗(On Diff #556102)

I assume we want to implement this TODO at higher optimization levels?

llvm/lib/Target/RISCV/RISCVGISel.td
34 ↗(On Diff #556102)

Do we plan on implementing this earlier in a future patch? Why do we want to canonicalize it earlier?

llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll
36 ↗(On Diff #556102)

I think this should be slliw?

412 ↗(On Diff #556102)

Why is this instruction emitted? This looks like a nop.

We may want to handle this in a later pass.

llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/alu-rv32.mir
3 ↗(On Diff #556102)

Do we want tests for and_i64, or_i64 and xor_i64? And immediate variants for these and add_i64, sub_i64?

craig.topper added inline comments.Sep 15 2023, 12:04 PM
llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll
36 ↗(On Diff #556102)

The upper bits aren’t used by the sraiw so SLLI should be ok

412 ↗(On Diff #556102)

The upper half immediate is 0 and we don’t have combines enabled.

llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/alu-rv32.mir
3 ↗(On Diff #556102)

I don’t think they provide any value. They would all be split instructions.

This revision is now accepted and ready to land.Sep 15 2023, 12:18 PM
craig.topper added inline comments.Sep 15 2023, 3:27 PM
llvm/lib/Target/RISCV/RISCVGISel.td
34 ↗(On Diff #556102)

I was planning to add a post legalizer combine for it.

craig.topper added inline comments.Sep 15 2023, 3:38 PM
llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll
36 ↗(On Diff #556102)

To clarify, the change from slliw to slli was made by the RISCVOptWInstrs pass.

This revision was landed with ongoing or failed builds.Sep 15 2023, 3:50 PM
This revision was automatically updated to reflect the committed changes.