ExpandPostRAPseudos should transfer implicit uses, not only implicit defs

[mkuper]

Summary

This fixes PR28560.

ExpandPostRAPseudos would transform:

%BL<def> = COPY %DL<kill>, %EBX<imp-use,kill>, %EBX<imp-def>

Into:

%BL<def> = MOV8rr %DL<kill>, %EBX<imp-def>

That caused problems for CriticalAntiDepBreaker which - I believe, correctly - assumes that if an instruction defs but doesn't use a register, that register was dead immediately before the instruction. However, in our case, the high part of EBX is very much alive.

Two notes about the tests:

1. The new instruction in the arm test isn't really new, it's a scheduling change. The vmov used to be immediately before the vld1, instead of after. Unfortunately, I don't understand ARM enough (= at all) to judge whether this is significant.
2. The regalloc for the x86 test looks like it could be better - and I'm not entirely sure we need the mov at all. But that's a separate issue.

[echristo]

This makes complete sense to me. Letting Quentin or Matthias give an ack though.

-eric

[MatzeB]

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.

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.

lib/CodeGen/ExpandPostRAPseudos.cpp

72–77 ↗

(On Diff #64195)

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);
}

[mkuper]

In 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.

lib/CodeGen/ExpandPostRAPseudos.cpp

72–77 ↗

(On Diff #64195)

Right, thanks!

[MatzeB]

In D22425#486010, @mkuper wrote:

In 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.

MIR still has clunky syntax and if you are unlucky crashs, but it is certainly good enough for most situations nowadays.

> ls PowerPC/*.mir X86/*.mir AArch64/*.mir | wc -l
14

It worked for 14 real world tests already.

For cases like this I really like the fact that the test is not depend on isel and other unrelated passes which IMO even outweights the clunky syntax...

• Matthias

[MatzeB]

In D22425#486017, @MatzeB wrote:

In D22425#486010, @mkuper wrote:

In 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.

MIR still has clunky syntax and if you are unlucky crashs, but it is certainly good enough for most situations nowadays.

> ls PowerPC/*.mir X86/*.mir AArch64/*.mir | wc -l
14

It worked for 14 real world tests already.

For cases like this I really like the fact that the test is not depend on isel and other unrelated passes which IMO even outweights the clunky syntax...

• Matthias

That said I'm fine with keeping the .ll test here.

llvm/trunk/lib/CodeGen/ExpandPostRAPseudos.cpp

public:
 
   /// runOnMachineFunction - pass entry point
   bool runOnMachineFunction(MachineFunction&) override;
 
 private:
   bool LowerSubregToReg(MachineInstr *MI);
   bool LowerCopy(MachineInstr *MI);
 
-  void TransferImplicitDefs(MachineInstr *MI);
+  void TransferImplicitOperands(MachineInstr *MI);
 };
 } // end anonymous namespace
 
 char ExpandPostRA::ID = 0;
 char &llvm::ExpandPostRAPseudosID = ExpandPostRA::ID;
 
 INITIALIZE_PASS(ExpandPostRA, "postrapseudos",
                 "Post-RA pseudo instruction expansion pass", false, false)
 
-/// TransferImplicitDefs - MI is a pseudo-instruction, and the lowered
-/// replacement instructions immediately precede it.  Copy any implicit-def
+/// TransferImplicitOperands - MI is a pseudo-instruction, and the lowered
+/// replacement instructions immediately precede it.  Copy any implicit
 /// operands from MI to the replacement instruction.
-void
-ExpandPostRA::TransferImplicitDefs(MachineInstr *MI) {
+void ExpandPostRA::TransferImplicitOperands(MachineInstr *MI) {
   MachineBasicBlock::iterator CopyMI = MI;
   --CopyMI;
 
-  for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
-    MachineOperand &MO = MI->getOperand(i);
-    if (!MO.isReg() || !MO.isImplicit() || MO.isUse())
-      continue;
-    CopyMI->addOperand(MachineOperand::CreateReg(MO.getReg(), true, true));
-  }
+  for (const MachineOperand &MO : MI->implicit_operands())
+    if (MO.isReg())
+      CopyMI->addOperand(MO);
 }
 
 bool ExpandPostRA::LowerSubregToReg(MachineInstr *MI) {
   MachineBasicBlock *MBB = MI->getParent();
   assert((MI->getOperand(0).isReg() && MI->getOperand(0).isDef()) &&
          MI->getOperand(1).isImm() &&
          (MI->getOperand(2).isReg() && MI->getOperand(2).isUse()) &&
           MI->getOperand(3).isImm() && "Invalid subreg_to_reg");
if (SrcMO.getReg() == DstMO.getReg()) {
     return true;
   }
 
   DEBUG(dbgs() << "real copy:   " << *MI);
   TII->copyPhysReg(*MI->getParent(), MI, MI->getDebugLoc(),
                    DstMO.getReg(), SrcMO.getReg(), SrcMO.isKill());
 
   if (MI->getNumOperands() > 2)
-    TransferImplicitDefs(MI);
+    TransferImplicitOperands(MI);
   DEBUG({
     MachineBasicBlock::iterator dMI = MI;
     dbgs() << "replaced by: " << *(--dMI);
   });
   MI->eraseFromParent();
   return true;
 }
 

llvm/trunk/test/CodeGen/ARM/twoaddrinstr.ll

 ; Tests for the two-address instruction pass.
 ; RUN: llc -mtriple=arm-eabi -mcpu=cortex-a9 -arm-atomic-cfg-tidy=0 %s -o - | FileCheck %s
 
 define void @PR13378() nounwind {
 ; This was orriginally a crasher trying to schedule the instructions.
 ; CHECK-LABEL:      PR13378:
 ; CHECK:        vld1.32
+; CHECK-NEXT:   vmov.i32
 ; CHECK-NEXT:   vst1.32
 ; CHECK-NEXT:   vst1.32
 ; CHECK-NEXT:   vmov.f32
 ; CHECK-NEXT:   vmov.f32
 ; CHECK-NEXT:   vst1.32
 
 entry:
   %0 = load <4 x float>, <4 x float>* undef, align 4
   store <4 x float> zeroinitializer, <4 x float>* undef, align 4
   store <4 x float> %0, <4 x float>* undef, align 4
   %1 = insertelement <4 x float> %0, float 1.000000e+00, i32 3
   store <4 x float> %1, <4 x float>* undef, align 4
   unreachable
 }

llvm/trunk/test/CodeGen/X86/pr28560.ll

+; RUN: llc -mtriple=i686-pc-linux -print-after=postrapseudos < %s 2>&1 | FileCheck %s
+
+; CHECK: MOV8rr %{{[A-D]}}L, %E[[R:[A-D]]]X<imp-use,kill>, %E[[R]]X<imp-def>
+define i32 @foo(i32 %i, i32 %k, i8* %p) {
+  %f = icmp ne i32 %i, %k
+  %s = zext i1 %f to i8
+  %ret = zext i1 %f to i32
+  br label %next
+next:
+  %d = add i8 %s, 5
+  store i8 %d, i8* %p
+  ret i32 %ret
+}