[PATCH] D109300: [AMDGPU] Make vector superclasses allocatable

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2432
+      Idx == AMDGPU::RegisterPressureSets::AGPR_32 ||
+      Idx == AMDGPU::RegisterPressureSets::AV_32)
     return getRegPressureLimit(&AMDGPU::VGPR_32RegClass,
----------------
rampitec wrote:
> cdevadas wrote:
> > rampitec wrote:
> > > cdevadas wrote:
> > > > rampitec wrote:
> > > > > I do not think there can be a separate pressure on AV. What does that mean for AV pressure if you have pressure 256 on V and 256 on A? It cannot be increased or decreased separately.
> > > > The assert here https://github.com/llvm/llvm-project/blob/main/llvm/utils/TableGen/CodeGenRegisters.cpp#L2028
> > > > enforces at least one PSet when we make the AV classes allocatable.
> > > > 
> > > > There should have been a flag to entirely avoid the psets for allocatable regclasses.
> > > There is GeneratePressureSet to avoid it, but the issue is conceptual. I do not understand a strategy of tracking pressure for a combined class.
> > GeneratePressureSet doesn't help to avoid the PSet entirely. As you can see, for all AV classes except AV_32, GeneratePressureSet is currently zero. The moment I reset this flag for AV_32, it asserts as I indicated earlier.
> > 
> I did not see that assert because I was using optimized tablegen. Shall we omit the assert if GeneratePressureSet  = 0? On practice it passes testing with GeneratePressureSet = 0 and optimized tablegen.
Yes, it all worked well with no Psets generated for AV classes when I used the optimized tablegen.
I can post a patch to remove that assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109300