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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

Do you know how RA will chose registers for an AV operand? V, A, and AV seem to have same AllocationPriority, so what exactly RA will be doing? At least on gfx908 we would want it to pick least congested RC. Well, after allocating everything which truly requires either V or A.
In fact I believe it will do directly the opposite, given that wider tuples have higher priority, so RA will likely start with AV before any other allocation is considered.



================
Comment at: llvm/lib/Target/AMDGPU/GCNRegPressure.cpp:81
+             ? (STI->getRegSizeInBits(*RC) == 32 ? AGPR32 : AGPR_TUPLE)
+             : (STI->getRegSizeInBits(*RC) == 32 ? VGPR32 : VGPR_TUPLE);
 }
----------------
So for AV and VS it will always tell VGPR? It does not seem conceptually right.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:262
   MI.getOperand(0).setReg(CopyUse.getOperand(0).getReg());
-  bool IsAGPR = TRI->hasAGPRs(DstRC);
+  bool IsAGPR = TRI->isAGPRClass(DstRC);
 
----------------
Now that AV is allocatable I suspect we will miss a lot of optimizations with multiple changes like this (e.g. hasAGPRs -> isAGPRClass) and maybe even make wrong and illegal decisions because we will be unable to correctly identify if that is a VGPR or AGPR presented an AV.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:896
   unsigned Opcode = AMDGPU::V_MOV_B32_e32;
-  if (RI.hasAGPRs(RC)) {
+  if (RI.isAGPRClass(RC)) {
     Opcode = (RI.hasVGPRs(SrcRC)) ?
----------------
What will happen if that is AV?


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2432
+      Idx == AMDGPU::RegisterPressureSets::AGPR_32 ||
+      Idx == AMDGPU::RegisterPressureSets::AV_32)
     return getRegPressureLimit(&AMDGPU::VGPR_32RegClass,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109300