[PATCH] D62614: Fix for the OCL/LC to failure on some OCLPerf tests

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:4174-4180
+      unsigned Reg = Op.getReg();
+      MachineInstr *DefMI = MRI.getUniqueVRegDef(Reg);
+      if (DefMI && DefMI->getParent() != InsertBB &&
+          MDT->dominates(DefMI->getParent(), InsertBB)) {
+        InsertBB = DefMI->getParent();
+        Insert = InsertBB->getFirstTerminator();
+      }
----------------
Can you give an example of why this is necessary? Assuming we have LCSSA phis, I don't understand the reason for this.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:5969-5979
+bool SIInstrInfo::shouldSink(const MachineInstr &MI) const {
+  const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
+  if (MI.isCopy()) {
+    unsigned SrcReg = MI.getOperand(1).getReg();
+    unsigned DstReg = MI.getOperand(0).getReg();
+    if (RI.isSGPRReg(MRI, SrcReg) && !RI.isSGPRReg(MRI, DstReg)) {
+      return false;
----------------
shouldSink should never be used for correctness. See the comment:
```
  /// MachineSink determines on its own whether the instruction is safe to sink;
  /// this gives the target a hook to override the default behavior with regards
  /// to which instructions should be sunk.
```
I suspect the right fix is to ensure that COPY instructions inserted by legalizeGenericOperands are marked with an implicit use of EXEC when a VGPR is involved.


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

https://reviews.llvm.org/D62614