Page MenuHomePhabricator

[TableGen] Emit table mapping physical registers to base classes
ClosedPublic

Authored by critson on Dec 8 2022, 2:55 AM.

Details

Summary

Allow targets to define a mapping from registers to register
classes such that each register has exactly one base class.
As registers may be in multiple register classes the base class
is determined by the container class with the lowest BaseClassOrder.

Only register classes with BaseClassOrder set are considered
when determining the base classes. By default BaseClassOrder is
unset in RegisterClass so no code is generated unless a target
explicit defines one or more base register classes.

Diff Detail

Event Timeline

critson created this revision.Dec 8 2022, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 2:55 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
critson requested review of this revision.Dec 8 2022, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 2:55 AM
foad added inline comments.Dec 8 2022, 3:29 AM
llvm/utils/TableGen/RegisterInfoEmitter.cpp
1202

Comparing with nullopt is weird. Can't you rely on implicit conversion to bool?

1607

You're not using Order here.

1611

< UINT8_MAX should be enough.

1621

This is return std::tie(LHS->getBaseClassOrder(), LHS->EnumValue) < std::tie(RHS->getBaseClassOrder(), RHS->EnumValue)

1628

Why + 1?

1648

Put nullptr as the first entry in the table to avoid this if?

Thanks, it's nice to finally move some of this stuff into tablegen. Can you add a test, or at least the AMDGPU sample usage?

llvm/include/llvm/CodeGen/TargetRegisterInfo.h
696

Should elaborate on what base means here.

llvm/utils/TableGen/RegisterInfoEmitter.cpp
1628

Can do this directly in constructor

critson marked 8 inline comments as done.Dec 8 2022, 9:45 PM
critson added inline comments.
llvm/utils/TableGen/RegisterInfoEmitter.cpp
1611

Not if we use first element for nullptr (in revised version).

1621

std::tie cannot handle std::optional argument it seems, so I have done this with std::pair instead.

1628

+ 1 is for NoRegister which is not part of Regs. I have added a comment to document this.

critson updated this revision to Diff 481514.Dec 8 2022, 9:45 PM
critson marked 3 inline comments as done.
  • Address reviewer feedback
  • Add lit test
critson updated this revision to Diff 481520.Dec 8 2022, 10:33 PM
  • Fix trivial off by one error in base class table
foad added inline comments.Dec 9 2022, 6:56 AM
llvm/utils/TableGen/RegisterInfoEmitter.cpp
1611

It would work fine to emit BaseClasses[256], so BaseClasses.size() can be 255, but you're restricting it to a max value of 253 here, so it's an off-by-2 error.

1621

Comparing the std::optionals themselves is a bit silly when you have already checked that they both have a value. I would suggest just adding a *: std::tie(*LHS->getBaseClassOrder(), LHS->EnumValue) ...

1644

Just assert Reg < sizeof(Mapping). Needs a small fix in IsCopyFromSGPR in D139422.

critson updated this revision to Diff 481755.Dec 9 2022, 3:13 PM
  • Use assertion instead of range check when resolving base classes
critson marked 3 inline comments as done.Dec 9 2022, 5:35 PM
critson added inline comments.
llvm/utils/TableGen/RegisterInfoEmitter.cpp
1611

OK, yes, because of the comparison being less-than.

1621

I agree the optionals can be dereferenced, but we still cannot use std::tie as they resulting value is not an l-value. I really don't think it matters to use std::pair here (or std::tuple), this code is executed once during tablegen on a maximum of 254 values.

critson updated this revision to Diff 481807.Dec 9 2022, 5:36 PM
critson marked 2 inline comments as done.
  • Address reviewer comments
foad added inline comments.Dec 11 2022, 10:22 PM
llvm/utils/TableGen/RegisterInfoEmitter.cpp
1612

This is still off by one. UINT8_MAX is 255 not 256.

foad added inline comments.Dec 11 2022, 10:23 PM
llvm/utils/TableGen/RegisterInfoEmitter.cpp
1621

Ack. I had misunderstood std::tie.

arsenm accepted this revision.Dec 13 2022, 2:09 PM
This revision is now accepted and ready to land.Dec 13 2022, 2:09 PM
critson updated this revision to Diff 482646.Dec 13 2022, 3:32 PM
critson marked an inline comment as done.
  • Extend available base class range by one
foad accepted this revision.Dec 14 2022, 1:57 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Dec 19 2022, 10:46 PM
This revision was automatically updated to reflect the committed changes.