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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2169
 
-// TODO: It might be helpful to have some target specific flags in
-// TargetRegisterClass to mark which classes are VGPRs to make this trivial.
 bool SIRegisterInfo::hasVGPRs(const TargetRegisterClass *RC) const {
+  return RC->TSFlags & SIRCFlags::IsVGPR;
----------------
I think we need a comment here it only works with an allocatable class until we need to change it to work with combined classes.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.td:853
   let isAllocatable = 0;
+  let IsVGPR = 1;
 }
----------------
cdevadas wrote:
> 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? 
> 
> 
It might be beneficial to add an SGPR flag, but not necessarily now. I think all of that will need a revisit when a first combined class will become allocatable. For now a rename of flags shall be OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108815