PR47468: Fix findPHICopyInsertPoint, so that copies aren't incorrectly inserted after an INLINEASM_BR.

[jyknight]

Summary

findPHICopyInsertPoint special cases placement in a block with a
callbr or invoke in it. In that case, we must ensure that the copy is
placed before the INLINEASM_BR or call instruction, if the register is
defined prior to that instruction, because it may jump out of the
block.

Previously, the code placed it immediately after the last def _or
use_. This is wrong, if the use is the instruction which may jump. We
could correctly place it immediately after the last def (ignoring
uses), but that is non-optimal for register pressure.

Instead, place the copy after the last def, or before the
call/inlineasm_br, whichever is later.

[nickdesaulniers]

llvm/lib/CodeGen/PHIEliminationUtils.cpp

43

drop these extra parens?

45–58

Hoist above L34 and then reuse EHPadSuccessor?

52

if (DefsInMBB.contains(&*I)) { (I know it's the same impl...and the previous version used that method)

[efriedma]

llvm/lib/CodeGen/PHIEliminationUtils.cpp

57

Is it possible to have multiple INLINEASM_BR instructions in the same block? If it is, we need to find the first one, not the last one.

[void]

llvm/lib/CodeGen/PHIEliminationUtils.cpp

57

It shouldn't come to that. The callbr is a terminator, and an MBB with an INLINEASM_BR will have multiple successors, so there shouldn't be block merging.

[jyknight]

llvm/lib/CodeGen/PHIEliminationUtils.cpp

57

Yes, both here and in SplitKit.cpp, we assume that there can only be one. (Note that this is not a _new_ assumption, it was also long the case that we assume only one invoke may be in a MBB,)

I'm not 100% happy with that, but it's definitely be the case for all MIR lowered from IR, and the blocks won't be merged so that property will be preserved.

[jyknight]

Comment nits.

llvm/lib/CodeGen/PHIEliminationUtils.cpp

llvm::findPHICopyInsertPoint(MachineBasicBlock* MBB, MachineBasicBlock* SuccMBB,
                              unsigned SrcReg) {
   // Handle the trivial case trivially.
   if (MBB->empty())
     return MBB->begin();
 
   // Usually, we just want to insert the copy before the first terminator
   // instruction. However, for the edge going to a landing pad, we must insert
   // the copy before the call/invoke instruction. Similarly for an INLINEASM_BR
-  // going to an indirect target.
-  if (!SuccMBB->isEHPad() && !SuccMBB->isInlineAsmBrIndirectTarget())
+  // going to an indirect target. This is similar to SplitKit.cpp's
+  // computeLastInsertPoint, and similarly assumes that there cannot be multiple
+  // instructions that are Calls with EHPad successors or INLINEASM_BR in a
+  // block.
+  bool EHPadSuccessor = SuccMBB->isEHPad();
+  if (!EHPadSuccessor && !SuccMBB->isInlineAsmBrIndirectTarget())
     return MBB->getFirstTerminator();
 
-  // Discover any defs/uses in this basic block.
-  SmallPtrSet<MachineInstr*, 8> DefUsesInMBB;
+  // Discover any defs in this basic block.
+  SmallPtrSet<MachineInstr *, 8> DefsInMBB;
   MachineRegisterInfo& MRI = MBB->getParent()->getRegInfo();
-  for (MachineInstr &RI : MRI.reg_instructions(SrcReg)) {
+  for (MachineInstr &RI : MRI.def_instructions(SrcReg))
     if (RI.getParent() == MBB)
-      DefUsesInMBB.insert(&RI);
-  }
+      DefsInMBB.insert(&RI);
 
-  MachineBasicBlock::iterator InsertPoint;
-  if (DefUsesInMBB.empty()) {
-    // No defs.  Insert the copy at the start of the basic block.
-    InsertPoint = MBB->begin();
-  } else if (DefUsesInMBB.size() == 1) {
-    // Insert the copy immediately after the def/use.
-    InsertPoint = *DefUsesInMBB.begin();
-    ++InsertPoint;
-  } else {
-    // Insert the copy immediately after the last def/use.
-    InsertPoint = MBB->end();
-    while (!DefUsesInMBB.count(&*--InsertPoint)) {}
-    ++InsertPoint;
+  MachineBasicBlock::iterator InsertPoint = MBB->begin();
+  // Insert the copy at the _latest_ point of:
+  // 1. Immediately AFTER the last def
+  // 2. Immediately BEFORE a call/inlineasm_br.
+  for (auto I = MBB->rbegin(), E = MBB->rend(); I != E; ++I) {
+    if (DefsInMBB.contains(&*I)) {
+      InsertPoint = std::next(I.getReverse());
+      break;
+    }
+    if ((EHPadSuccessor && I->isCall()) ||
+        I->getOpcode() == TargetOpcode::INLINEASM_BR) {
+      InsertPoint = I.getReverse();
+      break;
+    }
   }
 
   // Make sure the copy goes after any phi nodes but before
   // any debug nodes.
   return MBB->SkipPHIsAndLabels(InsertPoint);
 }

llvm/test/CodeGen/X86/callbr-asm-phi-placement.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -O2 < %s | FileCheck %s
+
+;; https://bugs.llvm.org/PR47468
+
+;; PHI elimination should place copies BEFORE the inline asm, not
+;; after, even if the inline-asm uses as an input the same value as
+;; the PHI.
+
+declare void @foo(i8*)
+
+define void @test1(i8* %arg, i8** %mem) nounwind {
+; CHECK-LABEL: test1:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pushq %r14
+; CHECK-NEXT:    pushq %rbx
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    movq %rsi, %r14
+; CHECK-NEXT:  .Ltmp0: # Block address taken
+; CHECK-NEXT:  .LBB0_1: # %loop
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    movq (%r14), %rbx
+; CHECK-NEXT:    callq foo
+; CHECK-NEXT:    movq %rbx, %rdi
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:  # %bb.2: # %end
+; CHECK-NEXT:    addq $8, %rsp
+; CHECK-NEXT:    popq %rbx
+; CHECK-NEXT:    popq %r14
+; CHECK-NEXT:    retq
+entry:
+  br label %loop
+
+loop:
+  %a = phi i8* [ %arg, %entry ], [ %b, %loop ]
+  %b = load i8*, i8** %mem, align 8
+  call void @foo(i8* %a)
+  callbr void asm sideeffect "", "*m,X"(i8* %b, i8* blockaddress(@test1, %loop))
+          to label %end [label %loop]
+
+end:
+  ret void
+}