[PATCH] D22425: ExpandPostRAPseudos should transfer implicit uses, not only implicit defs

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

In https://reviews.llvm.org/D22425#485991, @MatzeB wrote:

> If BL is defined, it does not mean that the other lanes are dead before the instruction. So there is still a bug in the anti dependency breaker.


If I understand the code there correctly, the anti dependency breaker will not declare the super-register dead just because the sub-register is dead, it actually needs a def (or imp-def) of the super-register. So I'm not sure there's still a bug.

> On the other hand this patch is obviously good and there is no reason not to do it, so LGTM.

>  Have you considered writing a .mir test ("llc -run-pass postrapseudos ...")? That should give you a much more stable test.


Is MIR is stable enough to write tests *in* it, not just *for* it?
I'd go for that if I were unable to come up with an IR test, but since the IR test looks fairly sane, I'd prefer to keep it.


================
Comment at: lib/CodeGen/ExpandPostRAPseudos.cpp:72-77
@@ -71,8 +71,8 @@
 
   for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
     MachineOperand &MO = MI->getOperand(i);
-    if (!MO.isReg() || !MO.isImplicit() || MO.isUse())
+    if (!MO.isReg() || !MO.isImplicit())
       continue;
-    CopyMI->addOperand(MachineOperand::CreateReg(MO.getReg(), true, true));
+    CopyMI->addOperand(MachineOperand::CreateReg(MO.getReg(), MO.isDef(), true));
   }
 }
----------------
MatzeB wrote:
> If we are on it anyway we could clean this code up:
> ```
>   for (const MachineOperand &MO : MI->implicit_operands()) {
>     if (MO.isReg())
>       CopyMI->addOperand(MO);
>   }
> ```
> 
Right, thanks!


https://reviews.llvm.org/D22425