[PATCH] D105800: [AMDGPU] Tidy SReg/SGPR definitions using template class

[Carl Ritson via Phabricator via llvm-commits]

critson marked an inline comment as done.
critson added a comment.

In D105800#2871152 <https://reviews.llvm.org/D105800#2871152>, @foad wrote:

> 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.



================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.td:727
+
+    // FIXME: ideally would always be isAllocatable = 0,
+    // but that causes all TableGen-generated subclasses to be marked
----------------
critson wrote:
> foad wrote:
> > Should we be defining TTMP classes for all sizes instead of working around it like this?
> 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.
> Should we be defining TTMP classes for all sizes instead of working around it like this?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105800/new/

https://reviews.llvm.org/D105800