[PATCH] D78570: [AMDGPU] Use RegClass helper functions in getRegForInlineAsmConstraint.

Wed Apr 22 01:36:05 PDT 2020

foad marked 2 inline comments as done.
foad added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10597-10599
       case 64:
         RC = &AMDGPU::SGPR_64RegClass;
         break;
----------------
arsenm wrote:
> foad wrote:
> > I don't like having exceptions like this, where we override the class that getSGPRClassForBitWidth would return. There are similar things in e.g. SIRegisterInfo::getEquivalentSGPRClass.
> > 
> > Perhaps getSGPRClassForBitWidth et al should be split into two different helpers, "get allocatable class" and "get *something else* class". But I really don't understand the purpose and the naming of the SReg/VReg/AReg vs SGPR/VGPR/AGPR classes, so I'm not sure how to do that.
> For the VGPR and AGPR cases, I think it's a naming mistake based on how these are defined in tablegen. We ended up with the register list as VGPR_* and then the class as VReg_*s for > 32.
> 
> For SGPRs, there is a distinction. SReg_* is more inclusive. SGPR_* only include number SGPRs. The various SReg_*s include different combinations of special registers like m0 and vcc or ttmp
Thanks. Perhaps we should have both `getSGPRClassForBitWidth` and `getSRegClassForBitWidth`?

What about the TTMP classes? Should there be one for every SGPR class? Currently we have TTMP_32/64/128/256/512, missing 96/160/1024.

================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10636
+      if (BitWidth == 1024) {
         // v32 types are not legal but we support them here.
         return std::make_pair(0U, RC);
----------------
rampitec wrote:
> This comment is obsolete. We do support v32* since then. You probably do not need special case for 1024 here.
Thanks. I committed that separately and rebased this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78570