[PATCH] D89619: [AMDGPU][NFC] Tidy SIOptimizeExecMaskingPreRA for extensibility

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks fine to me. More ideas inline.



================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp:79
 
-static bool isFullExecCopy(const MachineInstr& MI, const GCNSubtarget& ST) {
-  unsigned Exec = ST.isWave32() ? AMDGPU::EXEC_LO : AMDGPU::EXEC;
-
-  if (MI.isFullCopy() && MI.getOperand(1).getReg() == Exec)
+static bool isFullExecCopy(const MachineInstr &MI, Register ExecReg) {
+  if (MI.isFullCopy() && MI.getOperand(1).getReg() == ExecReg)
----------------
Make this a member function so you don't have to pass ExecReg in? Or just inline it? It's so simple now, it hardly seems worth having it as a separate function.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp:80
+static bool isFullExecCopy(const MachineInstr &MI, Register ExecReg) {
+  if (MI.isFullCopy() && MI.getOperand(1).getReg() == ExecReg)
     return true;
----------------
Since you're cleaning up, why not just return this expression?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp:128
 // Returns %cc register on success.
-static unsigned optimizeVcndVcmpPair(MachineBasicBlock &MBB,
-                                     const GCNSubtarget &ST,
-                                     MachineRegisterInfo &MRI,
-                                     LiveIntervals *LIS) {
-  const SIRegisterInfo *TRI = ST.getRegisterInfo();
-  const SIInstrInfo *TII = ST.getInstrInfo();
-  bool Wave32 = ST.isWave32();
-  const unsigned AndOpc = Wave32 ? AMDGPU::S_AND_B32 : AMDGPU::S_AND_B64;
-  const unsigned Andn2Opc = Wave32 ? AMDGPU::S_ANDN2_B32 : AMDGPU::S_ANDN2_B64;
-  const unsigned CondReg = Wave32 ? AMDGPU::VCC_LO : AMDGPU::VCC;
-  const unsigned ExecReg = Wave32 ? AMDGPU::EXEC_LO : AMDGPU::EXEC;
-
+unsigned
+SIOptimizeExecMaskingPreRA::optimizeVcndVcmpPair(MachineBasicBlock &MBB) {
----------------
Return Register?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp:135
   if (I == MBB.terminators().end())
     return AMDGPU::NoRegister;
 
----------------
Could just return `Register()` here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89619