[PATCH] D117562: [AMDGPU] Sink immediate VGPR defs if high RP

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

Adding @foad, we need more eyes for this huge patch.

In a nutshell this is a rematerialization which shall be done by the RA. Although RA only does it when it needs to spill and does not take occupancy into account. Ideally it would be better to teach RA to do so via some sort of a target callback, but I guess it will not be easy.

Looking at the patch and given we cannot do it in RA itself I start to think that scheduler is better suited for this job, maybe as an extra last step. It already has our RPTracker and collected all that info, so I guess it shall be less code. Plus all of the sinking may be not needed if scheduler was able to improve the occupancy.



================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:222
       break;
+    case AMDGPU::V_MOV_B32_e32:
+      if (!IsVGPRDst || !I.getOperand(1).isImm() || !MRI->hasOneUse(Reg))
----------------
You need to check this mov does not have extra implicit operands, e.g. SIInstrInfo::isReallyTriviallyReMaterializable().


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:324
 
+    LLVM_DEBUG(dbgs() << "Immediate def only has one use, sinking: "
+                      << *VGPRImmDef
----------------
It is better to check it earlier, before adding to the list of sinkable registers.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:327
+                      << "    to its use before: " << *VGPRImmUse);
+    auto NewI = BuildMI(*VGPRImmUse->getParent(), Pos, DebugLoc(),
+                        TII->get(AMDGPU::V_MOV_B32_e32))
----------------
TII.reMaterialize()?


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:754
+    MachineInstr *Def = DefUseSinkPair.first;
+    Changed |= processReg(Def->getOperand(0).getReg());
+  }
----------------
Why delegate it to processReg() and not have all this code here? processReg() becomes really hard to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117562