[PATCH] D34130: [AMDGPU] Eliminate SGPR to VGPR copy when possible

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Typo in test file name



================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:182-187
+  unsigned DstReg = MI.getOperand(0).getReg();
+  unsigned SrcReg = Src.getReg();
+  if (!TargetRegisterInfo::isVirtualRegister(SrcReg) ||
+      !TargetRegisterInfo::isVirtualRegister(DstReg))
+    return false;
+
----------------
This probably breaks if you have a copy that looks like a sub register extract


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:192
+      continue;
+    if (MO.isDef() || UseMI->getParent() != MI.getParent() ||
+        UseMI->getOpcode() <= TargetOpcode::GENERIC_OP_END ||
----------------
Why do you check the parent?


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:193
+    if (MO.isDef() || UseMI->getParent() != MI.getParent() ||
+        UseMI->getOpcode() <= TargetOpcode::GENERIC_OP_END ||
+        !TII->isOperandLegal(*UseMI, &MO - &UseMI->getOperand(0), &Src))
----------------
Why do you need to check this? Isn't this filtering the global isel pseudos which should never exist here?


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:194
+        UseMI->getOpcode() <= TargetOpcode::GENERIC_OP_END ||
+        !TII->isOperandLegal(*UseMI, &MO - &UseMI->getOperand(0), &Src))
+      return false;
----------------
Avoid doing the weird pointer arithmetic?


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:198
+  // Change VGPR to SGPR destination.
+  MRI.setRegClass(DstReg, TRI->getEquivalentSGPRClass(MRI.getRegClass(DstReg)));
+  return true;
----------------
I forsee this breaking cases with multiple uses


================
Comment at: test/CodeGen/AMDGPU/opt-sgpr-to-vgrp-copy.ll:25
+}
+
+declare i32 @llvm.amdgcn.workitem.id.x() #0
----------------
Probably need a MIR test to stress multiple uses


Repository:
  rL LLVM

https://reviews.llvm.org/D34130