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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
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);
 
----------------
rampitec wrote:
> cdevadas wrote:
> > rampitec wrote:
> > > 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.
> > I agree we might miss out on certain optimizations. That should be handled (separately).
> > COPY is a special instruction, the machine opcode changes based on the target register for AMDGPU. 
> > Any Pre-RA copy instance with AV class, I am forcing it to the VGPR class.
> > 
> > But illegal decisions should be averted. If it happens, mostly occurs at the selection.
> > 
> The point of this code is to avoid unneeded copies between VGPRs and AGPRs. Forcing it to VGPRs misses the point.
Given no actual AV operand this is OK.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:4854
   // No VOP2 instructions support AGPRs.
-  if (Src0.isReg() && RI.isAGPR(MRI, Src0.getReg()))
+  if (Src0.isReg() && RI.hasAGPRs(RI.getRegClassForReg(MRI, Src0.getReg())))
     legalizeOpWithMove(MI, Src0Idx);
----------------
It is still isAGPR. No need to legalize VGPR.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:2789
+    const TargetRegisterClass *RC = RI.getRegClassForReg(*MRI, DstReg);
+    bool IsVectorRegCopy = RI.hasVectorRegisters(RC);
+    unsigned NewOpc =
----------------
cdevadas wrote:
> arsenm wrote:
> > This logically doesn't flow right. This shouldn't be checking hasVectorRegisters, and just use isVGPR. You override this decision later with the AGPR class
> That check was added to consider AV classes too. The opcode is later changed to V_ACCVGPR_WRITE_B32_e64 if t is AGPR only class.
You should not have any AV at this point, these all are resolved. Then it was working with AGPRs already. This portion does not seem to be needed.

However, reverting it revealed problem with global isel (llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx908 -o - -verify-machineinstrs < llvm/test/CodeGen/AMDGPU/ds_gws_align.ll):


```
  %13:av_32 = COPY %8:sreg_32
  %14:av_32 = COPY %9:sreg_32
```

You have w/a this by folding the immediate, but really global isel shall not produce AV operands.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:2420
   case AMDGPU::VGPR_HI16RegClassID:
+  case AMDGPU::AV_32RegClassID:
     return std::min(ST.getMaxNumVGPRs(Occupancy), ST.getMaxNumVGPRs(MF));
----------------
So again this is not that simple with a combined class. This is a lower bound, probably twice smaller than in reality. But I guess we cannot answer better.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:1430
 
-    if (TRI->isVGPR(*MRI, Reg)) {
+    if (TRI->hasVGPRs(TRI->getRegClassForReg(*MRI, Reg))) {
       const TargetRegisterClass *regClass =
----------------
It can only be VGPR here.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:1440
+      // Use VGPR regclass if it is an AV class.
+      if (Reg.isVirtual() && TRI->isVectorSuperClass(regClass))
+        MRI->setRegClass(Reg, TRI->getEquivalentVGPRClass(regClass));
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109300