[PATCH] D59295: [AMDGPU] Pre-allocate WWM registers to reduce VGPR pressure.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I really don't like introducing new, dynamically reserved registers for this. It's going to introduce hell for dealing with any kind of ABI, and reserved registers are generally a bad idea. There's also nothing guaranteeing there are any free registers available to reserve, since you are just grabbing totally unused ones. This is going to just hit some variant of the problem I've been working on solving for handling SGPR->VGPR spills. Can WWM code be moved into a bundle or something?



================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:1363-1365
 def int_amdgcn_wwm : Intrinsic<[llvm_any_ty],
-  [LLVMMatchType<0>], [IntrNoMem, IntrSpeculatable]
+  [LLVMMatchType<0>], [IntrNoMem, IntrSpeculatable, IntrConvergent]
 >;
----------------
This is a separate fix that can be split into its own patch


================
Comment at: lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp:52
+
+  StringRef getPassName() const override { return "SI Pre-allocate WWM Registers"; }
+
----------------
You can remove this


================
Comment at: lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp:222-227
+      if (MI.getOpcode() == AMDGPU::S_OR_SAVEEXEC_B64 &&
+          MI.getOperand(1).isImm() && MI.getOperand(1).getImm() == -1) {
+        LLVM_DEBUG(dbgs() << "entering WWM region: " << MI << "\n");
+        InWWM = true;
+        continue;
+      }
----------------
I don't like this hardcoded opcode check. Why is S_OR_SAVEEXEC_B64 special?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59295