[PATCH] D108815: [AMDGPU] Introduce RC flags for vector register classes

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.td:853
   let isAllocatable = 0;
+  let IsVGPR = 1;
 }
----------------
rampitec wrote:
> This is suspicious. I wonder why SIRegisterInfo::isVGPR() works. Probably because VS classes are unallocatable and it is called only after selection when we do not have unallocatable classes anymore.
> 
> Anyway, using this bit on a combined classes seems misleading. Maybe it shall be named 'HasVGPRs' instead?
> Anyway, using this bit on a combined classes seems misleading. Maybe it shall be named 'HasVGPRs' instead?
Yes, HasVGPRs flag makes more sense for the combined classes. I will update the patch.



================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.td:853
   let isAllocatable = 0;
+  let IsVGPR = 1;
 }
----------------
cdevadas wrote:
> rampitec wrote:
> > This is suspicious. I wonder why SIRegisterInfo::isVGPR() works. Probably because VS classes are unallocatable and it is called only after selection when we do not have unallocatable classes anymore.
> > 
> > Anyway, using this bit on a combined classes seems misleading. Maybe it shall be named 'HasVGPRs' instead?
> > Anyway, using this bit on a combined classes seems misleading. Maybe it shall be named 'HasVGPRs' instead?
> Yes, HasVGPRs flag makes more sense for the combined classes. I will update the patch.
> 
> This is suspicious. I wonder why SIRegisterInfo::isVGPR() works. Probably because VS classes are unallocatable and it is called only after selection when we do not have unallocatable classes anymore.

Right, it is possible that SIRegisterInfo::isVGPR() can identify it as an exclusive vgpr class. It is true even with the upstream compiler today. Maybe we don't have a test or a use case to catch it. The unallocatable classes do appear in the register-class query during selection. Do you think it is worth having another flag for SGPRs? 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108815