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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Is this relying on assumptions about how the placement of blocks after structurization? I think we need to augment the MIR to better track vector vs. scalar predecessors



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:168
+    cl::desc("Enable VGPR liverange optimizations for if-else structure"),
+    cl::init(false), cl::Hidden);
+
----------------
Should default to on


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:39
+///
+///  This pass aims to to tell LLVM that %a is in-fact dead, through inserting
+///  a phi-node in bb.flow saying that %a is undef when coming from bb.then,
----------------
s/LLVM/register allocator


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:72
+  const SIInstrInfo *TII = nullptr;
+  LiveVariables *LV;
+  MachineDominatorTree *MDT = nullptr;
----------------
Why no initializer like the rest?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:166
+  for (auto *MBB : Blocks) {
+    LLVM_DEBUG(dbgs() << " bb." << MBB->getNumber());
+  }
----------------
printName?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:168
+  }
+  LLVM_DEBUG(dbgs() << "\n");
+}
----------------
single quotes


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:203
+        Register MOReg = MO.getReg();
+        // We can only optimize VGPR virtual register
+        if (MOReg.isPhysical() || !TRI->isVGPR(*MRI, MOReg))
----------------
Could also handle AGPR


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:276
+
+  for (auto Reg : KillsInElse)
+    if (!IsLiveThroughThen(Reg))
----------------
Braces


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:301
+    // Clear Live bit, as we will recalculate afterwards
+    LLVM_DEBUG(dbgs() << "Clear AliveBlock bb." << MBB->getNumber() << "\n");
+    OldVarInfo.AliveBlocks.reset(MBB->getNumber());
----------------
printName


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:326
+    if (Uses.size() == 1) {
+      LLVM_DEBUG(dbgs() << "Found one Non-PHI use in bb." << MBB->getNumber()
+                        << "\n");
----------------
printName


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:459-460
+
+  if (skipFunction(MF.getFunction()))
+    return false;
+
----------------
Can move this before the initializations


================
Comment at: llvm/test/CodeGen/AMDGPU/vgpr-liverange.ll:3
+; RUN: llc -march=amdgcn -mcpu=tonga -amdgpu-opt-vgpr-liverange=true -stop-after=si-opt-vgpr-liverange -verify-machineinstrs < %s | FileCheck -check-prefix=SI %s
+
+; a normal if-else
----------------
Could really use an additional end to end IR checks to make sure this actually gets the allocator to do what you want


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212