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

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

In D102212#2764725 <https://reviews.llvm.org/D102212#2764725>, @arsenm wrote:

> 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

Thanks for the careful review! This pass assume the IR is structurized, mainly things around if-else-endif, the SI_ELSE should have a corresponding SI_IF and SI_ENDIF, and the sub-regions should be single-entry-single-exit, I don't think I have dependency on specific order of the blocks.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:168
+    cl::desc("Enable VGPR liverange optimizations for if-else structure"),
+    cl::init(false), cl::Hidden);
+
----------------
arsenm wrote:
> Should default to on
default on is ok to me, but I really hope we could do some testing at Compute and Mesa side to make sure we don't have obvious regression before landed.


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


================
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))
----------------
arsenm wrote:
> Could also handle AGPR
I guess AGPRs are used as physical registers? if yes, I think we cannot handle them here, as handling physical register needs lots more work regarding the live-or-not-checking and updating the LiveVariable information.


================
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
----------------
arsenm wrote:
> Could really use an additional end to end IR checks to make sure this actually gets the allocator to do what you want
good idea, will do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212