[PATCH] D23408: AMDGPU/SI: Avoid creating unnecessary copies in the SIFixSGPRCopies pass

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2289
@@ +2288,3 @@
+                                         MachineRegisterInfo &MRI,
+                                         DebugLoc DL) const {
+
----------------
DebugLocs are supposed to be passed by reference now

================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2291
@@ +2290,3 @@
+
+  // Check if operand is already the correct register class
+  if (DstRC == RI.getRegClassForReg(MRI, Op.getReg()))
----------------
Period

================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2299-2306
@@ +2298,10 @@
+
+  MachineInstr *Def = MRI.getVRegDef(Op.getReg());
+  Op.setReg(DstReg);
+  if (!Def)
+    return;
+
+  // Try to eliminate the copy if it is copying an immediate value.
+  if (Def->isMoveImmediate())
+    FoldImmediate(*Copy, *Def, Copy->getOperand(1).getReg(), &MRI);
+}
----------------
There are two different ways here of referring to the same, original value. It would be easier to follow if Op.getReg() was assigned to a variable at the beginning, and then consistently used instead of switching to Copy->getOperand(1).getReg(), and then you wouldn't need to worry about the order of the Op.setReg().

Could Op have a sub register index?

================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2755
@@ -2737,1 +2754,3 @@
       unsigned DstReg = Inst.getOperand(0).getReg();
+      if (Inst.getOpcode() == AMDGPU::COPY &&
+          TargetRegisterInfo::isVirtualRegister(Inst.getOperand(1).getReg()) &&
----------------
You can use Inst.isCopy()


https://reviews.llvm.org/D23408