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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
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;
+
----------------
arsenm wrote:
> This probably breaks if you have a copy that looks like a sub register extract
The should not be a problem. I'm changing VGPR class to equivalent SGPR class, they support the same set of subregs.


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:192
+      continue;
+    if (MO.isDef() || UseMI->getParent() != MI.getParent() ||
+        UseMI->getOpcode() <= TargetOpcode::GENERIC_OP_END ||
----------------
arsenm wrote:
> Why do you check the parent?
A copy can be in a divergent control flow. If it is the same block we can be sure that is actually the value we are going to use.


================
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))
----------------
arsenm wrote:
> Why do you need to check this? Isn't this filtering the global isel pseudos which should never exist here?
isOperandLegal does not work with generic opcodes because they have no RC classes assigned to operands.


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


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:198
+  // Change VGPR to SGPR destination.
+  MRI.setRegClass(DstReg, TRI->getEquivalentSGPRClass(MRI.getRegClass(DstReg)));
+  return true;
----------------
arsenm wrote:
> I forsee this breaking cases with multiple uses
Why? I'm checking all uses for operand legality. It also does not happen if there is a second def of the same register.


Repository:
  rL LLVM

https://reviews.llvm.org/D34130