This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] generate `getRegBankFromRegClass`
AbandonedPublic

Authored by paperchalice on Apr 3 2021, 2:59 AM.

Details

Summary

Generate getRegBankFromRegClass, referenced from ARM implementation. Currently LLT Ty is unused.

Diff Detail

Event Timeline

paperchalice created this revision.Apr 3 2021, 2:59 AM
paperchalice requested review of this revision.Apr 3 2021, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2021, 2:59 AM
paperchalice removed subscribers: sdardis, hiraditya, jrtc27 and 4 others.
paperchalice abandoned this revision.Apr 3 2021, 3:53 AM

Do you plan to revive this at some point? It would be great to move more of the regbank boilerplate from C++ to TableGen

Raise warning when two register banks are overlapped.

Try remove some hand-write implementations.

Failed Tests (4):

cfi-devirt-lld-thinlto-x86_64 :: icall/bad-signature.c
cfi-devirt-lld-x86_64 :: icall/bad-signature.c
cfi-standalone-lld-thinlto-x86_64 :: icall/bad-signature.c
cfi-standalone-lld-x86_64 :: icall/bad-signature.c
paperchalice updated this revision to Diff 335705.EditedApr 6 2021, 8:18 PM

Keep X86 implementation. Because 4 lld tests failed.

Update patch with full context.

When I build llc with this patch, with AMDGPU as a target, I get a lot of warnings.

E.g.

Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_128' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^
Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_160' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^

Is that expected?

When I build llc with this patch, with AMDGPU as a target, I get a lot of warnings.

E.g.

Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_128' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^
Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_160' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^

Is that expected?

AV_* classes are for operands that can be one of two different regbanks. You can't unambiguously go back from the class to the register bank. At the moment these are unallocatable classes, although this will probably be changing soon. I'm not really sure what this code should do in this case.

paquette added inline comments.Apr 13 2021, 11:17 AM
llvm/lib/Target/AArch64/GISel/AArch64RegisterBankInfo.cpp
266

This register class is missing from the table-generated version of this function in AArch64.

When I build llc with this patch, with AMDGPU as a target, I get a lot of warnings.

E.g.

Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_128' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^
Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_160' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^

Is that expected?

AV_* classes are for operands that can be one of two different regbanks. You can't unambiguously go back from the class to the register bank. At the moment these are unallocatable classes, although this will probably be changing soon. I'm not really sure what this code should do in this case.

Saying these are AGPR bank is definitely the wrong answer though. If it's going to be wrong, just saying VGPR would be a better choice

add missing WSeqPairsClass, XSeqPairsClass for GPRRegBank in AArch64

RKSimon added inline comments.Apr 14 2021, 7:54 AM
llvm/utils/TableGen/RegisterBankEmitter.cpp
305

QualifiedBankID is only used here - why create a std::string when you could just pass each of the the components to OS?

RKSimon added inline comments.Apr 16 2021, 5:46 AM
llvm/lib/Target/AArch64/GISel/AArch64RegisterBankInfo.cpp
266

@paperchalice Have you been able to move these remaining classes to tablegen yet?

paperchalice added inline comments.Apr 16 2021, 6:10 AM
llvm/lib/Target/AArch64/GISel/AArch64RegisterBankInfo.cpp
266

Some tests failed if changing def GPRRegBank : RegisterBank<"GPR", [GPR64all]>; to def GPRRegBank : RegisterBank<"GPR", [GPR64all, WSeqPairsClass, XSeqPairsClass]>;, because the code assume GPR is 64-bit, but the size of XSeqPairsClass is 128.

paperchalice abandoned this revision.Apr 19 2021, 10:42 PM