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

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

Thanks for all the careful comments, I will also address new comments from Carl and Jay in next version.



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


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:193
+  for (auto *Else : ElseBlocks) {
+    for (auto &MI : Else->instrs()) {
+      if (MI.isDebugInstr())
----------------
critson wrote:
> Is this Else->instrs() to skip phis for a reason? (Rather than *Else.)
I think they are just the same. instrs() means all the instructions I think.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:196
+        continue;
+      unsigned NumOps = MI.getNumOperands();
+      for (unsigned Op = 0; Op < NumOps; ++Op) {
----------------
foad wrote:
> critson wrote:
> > 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.
> Or `for (auto &MO : MI.operands())`?
sounds good.


================
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()) {
----------------
critson wrote:
> Where does this loop check that the registers added to KillsInElse are actually from the else branch?
I am assuming the predecessor that is not the Flow is from Else region. Maybe I need to add some assert for this.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:237
+
+      if (Reg.isPhysical() || !TRI->isVGPR(*MRI, Reg))
+        continue;
----------------
critson wrote:
> Should this test be before the live interval retrieval?
Do you mean put this check before the line "LV->getVarInfo(Reg)"? If yes, I will fix it next version.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212