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

[Carl Ritson via Phabricator via llvm-commits]

critson added a comment.

Sorry a few more comments, mostly minor.



================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:119
+    AU.addRequired<LiveVariables>();
+    AU.addRequired<MachineLoopInfo>();
+    AU.addPreserved<MachineDominatorTree>();
----------------
Order of Required and Preserved could be the same?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:152
+  unsigned Cur = 0;
+  while (MBB != nullptr) {
+    for (auto *Pred : MBB->predecessors()) {
----------------
I think this can just be "while (MBB) {"


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:158
+
+    if (Cur < Blocks.size())
+      MBB = Blocks[Cur++];
----------------
This doesn't seem right.
You only collect an arbitrary number of blocks?
Should ElseBlocks not simply be growing to hold all relevant blocks?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:197
+      for (auto &MO : MI.operands()) {
+        if (!MO.isReg() || MO.getReg() == AMDGPU::NoRegister || MO.isDef())
+          continue;
----------------
Instead of "MO.getReg() == AMDGPU::NoRegister", I believe you can just write "!MO.getReg()".


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:230
+
+      if (!MO.isReg() || MO.getReg() == AMDGPU::NoRegister || MO.isUndef())
+        continue;
----------------
As above.


================
Comment at: llvm/test/CodeGen/AMDGPU/vgpr-liverange2.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -march=amdgcn -mcpu=tonga -amdgpu-opt-vgpr-liverange=true -verify-machineinstrs < %s | FileCheck -check-prefix=SI %s
----------------
These files should have a better name.
Perhaps:
vgpr-liverange.ll -> vgpr-liverange-ir.ll
vgpr-liverange2.ll -> vgpr-liverange.ll

We definitely don't use numeric suffixes at present.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212