[PATCH] D102212: [AMDGPU] Add Optimize VGPR LiveRange Pass.

[Carl Ritson via Phabricator via llvm-commits]

critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:10
+/// \file
+/// This pass tries to optimize off unnecessary VGPR live range in divergent
+/// if-else structure.
----------------
"optimize off unnecessary VGPR live range" -> "remove unnecessary VGPR live ranges"?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:190
+
+  SmallSet<Register, 8> KillsInElse;
+
----------------
Any reason for explicit 8 here?
(I am also being reminded not to write explicit sizes in these.)


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:193
+  for (auto *Else : ElseBlocks) {
+    for (auto &MI : Else->instrs()) {
+      if (MI.isDebugInstr())
----------------
Is this Else->instrs() to skip phis for a reason? (Rather than *Else.)


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:196
+        continue;
+      unsigned NumOps = MI.getNumOperands();
+      for (unsigned Op = 0; Op < NumOps; ++Op) {
----------------
I think you can just use MI.getNumOperands() directly in the loop condition and leave it up to the compiler whether to hoist it?
In fact you do that in the next similar loop.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:199
+        MachineOperand &MO = MI.getOperand(Op);
+        if (!MO.isReg() || MO.getReg() == 0 || MO.isDef())
+          continue;
----------------
What is the "MO.getReg() == 0" for? It does not seem correct.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:222
+  // Check the phis in the Endif, looking for value coming from the ELSE
+  // region. Make sure the phi-use is the last use.
+  for (auto &MI : Endif->phis()) {
----------------
Where does this loop check that the registers added to KillsInElse are actually from the else branch?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:237
+
+      if (Reg.isPhysical() || !TRI->isVGPR(*MRI, Reg))
+        continue;
----------------
Should this test be before the live interval retrieval?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:290
+    if (Succ != Flow)
+      ThenEntry = Succ;
+  }
----------------
Can break out of loop here?
Also perhaps assert(ThenEntry) after loop?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:347
+  // Set the isKilled flag if we get new Kills in the THEN region.
+  for (auto *MI : OldVarInfo.Kills)
+    if (llvm::find(Visited, MI->getParent()) != Visited.end())
----------------
As Matt noted on the earlier one of these probably should use braces here too.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:371
+  std::vector<MachineInstr *>::iterator I = OldVarInfo.Kills.begin();
+  for (; I != OldVarInfo.Kills.end();) {
+    auto *KillBB = (*I)->getParent();
----------------
while?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:475
+
+        SmallVector<MachineBasicBlock *, 8> ElseBlocks;
+        SmallVector<Register, 8> CandidateRegs;
----------------
As earlier, do we need explicit 8s here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212