This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][GlobalISel] Select register banks for GPR ALU instructions
ClosedPublic

Authored by craig.topper on Mar 12 2020, 2:23 AM.

Details

Summary

This patch implements the getInstrMapping hook for RISCVRegisterBankInfo and others in order to correctly select the GPR register bank for operands of ALU instructions, and the associated operations introduced by the legalizer.

Diff Detail

Event Timeline

lewis-revill created this revision.Mar 12 2020, 2:23 AM
simoncook added inline comments.Mar 13 2020, 4:02 PM
llvm/lib/Target/RISCV/RISCVRegisterBankInfo.cpp
15

This looks strange this way round, does this still build if you swap lines 14 and 15?

98

reigsters -> registers, nitpick: RV32

lewis-revill retitled this revision from [RISCV][GlobalISel] Select register banks for GPR ALU instructions to [WIP][RISCV][GlobalISel] Select register banks for GPR ALU instructions.

Address comments

Support AND/OR/XOR.

Use utils/update_mir_test_checks.py

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

Expand patch to include operations for other types as introduced by the legalizer.

Add more operand mappings and expand tests over more types.

Bug fix - don't try to access the size of a $noreg register.

lenary resigned from this revision.Jan 14 2021, 10:10 AM
rkruppe removed a subscriber: rkruppe.Jan 14 2021, 10:20 AM

Rebased. Updated tests to match the output of the legalizer.

arsenm added inline comments.Jan 17 2022, 4:06 PM
llvm/lib/Target/RISCV/RISCVRegisterBankInfo.cpp
95

I'd recommend keeping the subtarget cached in the class

99

I don't see the point of this loop. It seems like you're just doing a rough legality check, but the function is already required (and enforced by the verifier) to be legal at this point

102

You shouldn't see noreg on a generic instruction so I'm not sure what you're addressing here

llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir
8

You can drop the IR section

Remove unnecessary loop checking legality

lewis-revill marked 4 inline comments as done.Feb 1 2022, 7:19 AM
lewis-revill retitled this revision from [WIP][RISCV][GlobalISel] Select register banks for GPR ALU instructions to [RISCV][GlobalISel] Select register banks for GPR ALU instructions.Feb 9 2022, 3:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 3:52 PM
evandro removed a subscriber: evandro.Aug 17 2023, 5:08 PM
craig.topper commandeered this revision.Aug 17 2023, 9:11 PM
craig.topper edited reviewers, added: lewis-revill; removed: craig.topper.

Re-add tests

Harbormaster completed remote builds in B253383: Diff 551369.

Use size from LLT instead of from Subtarget.getXLen. This will make it easier to support D157770.

nitinjohnraj added inline comments.Aug 18 2023, 3:37 PM
llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp
123 ↗(On Diff #551388)

Should we consider adding new entries to ValueMappings so that we don't create this array locally? All the binary operations above don't create local arrays for this (though they could).

131 ↗(On Diff #551388)

Should we consider adding new entries to ValueMappings so that we don't create this array locally? All the binary operations above don't create local arrays for this (though they could).

llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir
2

Missing test for G_CONSTANT and G_ICMP

7

This test is identical to add_i32 below.

52

I don't think we need this test since we're separately testing for shl and ashr.

79

We don't have a test for G_CONSTANT, but if we make one, we may not need this test. We have tests for G_AND.

287

It's impossible to get a legal G_MUL on RISCV unless the +m or +zmmul attributes are enabled. It might be fine to have this test here to simply demonstrate how we select register banks for G_MUL.

385

I'm not sure we need separate tests for _i64.

llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir
2

Missing test for G_CONSTANT and G_ICMP

7

Duplicate test, see add_i64

44

I don't think we need this test since we're separately testing for shl and ashr.

79

Once we have a test for G_CONSTANT, we may not need this test.

86

All the _i32 tests are duplicates of the _i64 tests.

665

I don't think that the _i128 tests are meaningful, since we test for each of these generic opcodes separately.

craig.topper added inline comments.Aug 18 2023, 4:05 PM
llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp
123 ↗(On Diff #551388)

This implementation is consistent with ARM, AArch64, and Mips. I don't think it is possible to put a nullptr entry in the table. It needs to be a null RegisterBankInfo::ValueMapping* but the table row is an array of RegisterBankInfo::ValueMapping.

Address review comments

Add G_SEXT_INREG

Change back to using Subtarget to get register size. I think even with s32 being legal
on RV64 we still want to use a 64 bit size for the GPR.

clang-format

Add more instructions that we currently legalize.

Use the size of the GPR register bank to determine RV32 vs RV64.

nitinjohnraj accepted this revision.Aug 22 2023, 11:35 AM

LGTM

llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/global-rv64.mir
7 ↗(On Diff #551866)

Should this global be an i64?

This revision is now accepted and ready to land.Aug 22 2023, 11:35 AM
craig.topper added inline comments.Aug 22 2023, 11:37 AM
llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/global-rv64.mir
7 ↗(On Diff #551866)

The type doesn't matter. We're interested in the pointer to the variable not the type of the variable itself. The variable could be any type, even FP or a struct/array.

This revision was landed with ongoing or failed builds.Aug 22 2023, 12:40 PM
This revision was automatically updated to reflect the committed changes.