This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Tidy SReg/SGPR definitions using template class
ClosedPublic

Authored by critson on Jul 12 2021, 1:41 AM.

Details

Summary

Use a multiclass to consistently define SReg/SGPR/TTMP register classes.
Add missing TTMP registers for 96b, 160b, 192b, 224b.

Diff Detail

Event Timeline

critson created this revision.Jul 12 2021, 1:41 AM
critson requested review of this revision.Jul 12 2021, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 1:41 AM
foad added a comment.Jul 12 2021, 7:37 AM

Looks like a nice refactoring.

llvm/lib/Target/AMDGPU/SIInstructions.td
1242–1243 ↗(On Diff #357861)

Was SGPR_96 some kind of anomaly among the existing classes?

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
700

Don't you want to divide by two rounding up, so !sra(!add(numRegs, 1), 1)?

727

Should we be defining TTMP classes for all sizes instead of working around it like this?

arsenm added inline comments.Jul 12 2021, 7:52 AM
llvm/lib/Target/AMDGPU/SIInstructions.td
1242–1243 ↗(On Diff #357861)

I think the *GPR_* class names are better where applicable

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
733

Typo inherence

critson updated this revision to Diff 358203.Jul 13 2021, 2:23 AM
critson marked 4 inline comments as done.
  • Address reviewer comments
critson added inline comments.Jul 13 2021, 2:25 AM
llvm/lib/Target/AMDGPU/SIInstructions.td
1242–1243 ↗(On Diff #357861)

OK, I've put these back. Pretty much everything else in this file uses SReg_*

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
700

Thanks.

727

At the moment if I create the TTMP_96Regs set then SGPR_128 stops working correctly. So I need to investigate further if we want to do that. Also SGPR_1024 will never have an associated TTMP set because there are only 16 trap registers.

critson marked an inline comment as done.Jul 13 2021, 7:55 PM

Should we be defining TTMP classes for all sizes instead of working around it like this?

I don't think we can at present.

If I define TTMP_96Regs this implicitly causes TTMP_128_with_sub0_sub1_sub2 to be created.
This then creates SReg_128_with_sub0_sub1_sub2.
Now SGPR_128_with_sub0_sub1_sub2 inherits from SReg_128_with_sub0_sub1_sub2 instead of from SGPR_128.
(The superclass of SReg_128_with_sub0_sub1_sub2 is SReg_128, but the superclasses of SGPR_128_with_sub0_sub1_sub2 are SReg_128, SGPR_128, SReg_128_with_sub0_sub1_sub2.)
The net result is that SGPR_128_with_sub0_sub1_sub2 is marked isAllocatable = 0, causing allocation new failures.

The bottom line is that I don't think isAllocatable = 0 currently supports what we are trying to do.
Best guess is it is only appropriate for isolated or leaf register classes.
I can pursue a TableGen change to try and address this, but it would be good to get the immediate issue of allocation failures with SGPR_192 fixed.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
727

Should we be defining TTMP classes for all sizes instead of working around it like this?

critson updated this revision to Diff 359212.Jul 15 2021, 9:45 PM
  • Rework based on TableGen changes in D105967
  • Define all applicable TTMP register classes
critson edited the summary of this revision. (Show Details)Jul 15 2021, 9:45 PM
foad accepted this revision.Jul 16 2021, 5:03 AM

Looks good, just nits inline.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
699

Wouldn't it be a bit simpler to use [] as the default, and test it with !empty?

700

Isn't defvar a better way of defining all these variables, instead of making them optional arguments that are never actually used?

This revision is now accepted and ready to land.Jul 16 2021, 5:03 AM
critson marked 2 inline comments as done.Jul 16 2021, 7:12 PM
critson added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
699

It's not possible to initialise a SIRegisterTuples variable with [], so this is the simplest solution in my opinion. Beyond creating a dummy SIRegisterTuples variable to use.

700

Good point. It seems we don't actually use defvar much in AMDGPU.

critson updated this revision to Diff 359517.Jul 16 2021, 7:12 PM
critson marked 2 inline comments as done.
  • Address nits
  • Fix allocation priority of SReg_224
This revision was landed with ongoing or failed builds.Jul 16 2021, 7:27 PM
This revision was automatically updated to reflect the committed changes.