[CodeGen] Postprocess PHI nodes for callbr

[void]

Summary

When processing PHI nodes after a callbr, resolve the PHI nodes on
the default branch are inserted after the callbr (inserted after
INLINEASM_BR). The PHI node values on the indirect branches are
inserted before the INLINEASM_BR.

[nickdesaulniers]

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

10

What was the input C program, was it precisely https://godbolt.org/z/zqK1K7 from https://bugs.llvm.org/show_bug.cgi?id=47242. If so, this mov into %eax looks problematic; it gets overwritten on both branches of the INLINEASM_BR, and looks like a dead store to me.

[craig.topper]

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

10

I think its there because the value of @var is an input to the inline asm. But the asm doesn't use it?

[nickdesaulniers]

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

10

ah, I think removing thread_local from @var may remove this noise from the test case.

[void]

Remove extraneous thread_local from global variable.

[void]

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

10

Yeah, it was identical to the GodBolt code. I removed the thread_local.

[jyknight]

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

[void]

In D86260#2229387, @jyknight wrote:

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

bb1:
  %eax = MOV 0
  INLINEASM_BR "mov 37, %eax"
  JMP return

return:
  PHI bb1, 0 ...

[jyknight]

In D86260#2229406, @void wrote:

In D86260#2229387, @jyknight wrote:

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

Inline assembly must declare what registers it clobbers, and the compiler can safely assume other registers are preserved across it.

bb1:
  %eax = MOV 0
  INLINEASM_BR "mov 37, %eax"
  JMP return

return:
  PHI bb1, 0 ...

So, in this example the input inline-asm is invalid, because it failed to declare that it clobbered eax.

[void]

In D86260#2229449, @jyknight wrote:

In D86260#2229406, @void wrote:

In D86260#2229387, @jyknight wrote:

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

Inline assembly must declare what registers it clobbers, and the compiler can safely assume other registers are preserved across it.

bb1:
  %eax = MOV 0
  INLINEASM_BR "mov 37, %eax"
  JMP return

return:
  PHI bb1, 0 ...

So, in this example the input inline-asm is invalid, because it failed to declare that it clobbered eax.

Ah! That's right. I forgot about the clobber list. I'll change the description.

[jyknight]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

10007

This seems a bit confusing.

I think it would probably be clearer to make the body of this loop into a separate function which handles a single successor BasicBlock. Call that function directly on the default successor from visitCallBr, and move the loop over successors with the callbr default-successor-exclusion into visit().

10082

This is problematical if we're calling this function multiple times now. A constant will be emitted redundantly, if it's present both in indirect and default successor phi branches. Oddly enough, this is a member variable instead of a local, even though it's only used in this function. I think you can just move this call to clear() to the end of visit(), and that solves the problem.

[efriedma]

The idea here is that we want to reduce the live-range of values used in the default destination? Makes sense as an optimization.

We could theoretically do this after isel, I think, but probably harder to implement.

[void]

Erm...Reverted...Sorry.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

private:
   }
   void visitUserOp2(const Instruction &I) {
     llvm_unreachable("UserOp2 should not exist at instruction selection time!");
   }
 
   void processIntegerCallValue(const Instruction &I,
                                SDValue Value, bool IsSigned);
 
-  void HandlePHINodesInSuccessorBlocks(const BasicBlock *LLVMBB);
+  void HandlePHINodesInSuccessorBlocks(const BasicBlock *LLVMBB,
+                                       bool Preprocess);
 
   void emitInlineAsmError(const CallBase &Call, const Twine &Message);
 
   /// If V is an function argument then create corresponding DBG_VALUE machine
   /// instruction for it now. At the end of instruction selection, they will be
   /// inserted to the entry BB.
   bool EmitFuncArgumentDbgValue(const Value *V, DILocalVariable *Variable,
                                 DIExpression *Expr, DILocation *DL,

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

SDValue SelectionDAGBuilder::getControlRoot() {
   PendingExports.append(PendingConstrainedFPStrict.begin(),
                         PendingConstrainedFPStrict.end());
   PendingConstrainedFPStrict.clear();
   return updateRoot(PendingExports);
 }
 
 void SelectionDAGBuilder::visit(const Instruction &I) {
   // Set up outgoing PHI node register values before emitting the terminator.
-  if (I.isTerminator()) {
-    HandlePHINodesInSuccessorBlocks(I.getParent());
-  }
+  if (I.isTerminator())
+    HandlePHINodesInSuccessorBlocks(I.getParent(), true);
 
   // Increase the SDNodeOrder if dealing with a non-debug instruction.
   if (!isa<DbgInfoIntrinsic>(I))
     ++SDNodeOrder;
 
   CurInst = &I;
 
   visit(I.getOpcode(), I);
void SelectionDAGBuilder::visitCallBr(const CallBrInst &I) {
   assert(!I.hasOperandBundlesOtherThan(
              {LLVMContext::OB_deopt, LLVMContext::OB_funclet}) &&
          "Cannot lower callbrs with arbitrary operand bundles yet!");
 
   assert(I.isInlineAsm() && "Only know how to handle inlineasm callbr");
   visitInlineAsm(I);
   CopyToExportRegsIfNeeded(&I);
 
+  // Set up outgoing PHI node register values before emitting the branch.
+  HandlePHINodesInSuccessorBlocks(I.getParent(), false);
+
   // Retrieve successors.
   MachineBasicBlock *Return = FuncInfo.MBBMap[I.getDefaultDest()];
 
   // Update successor info.
   addSuccessorWithProb(CallBrMBB, Return, BranchProbability::getOne());
   for (unsigned i = 0, e = I.getNumIndirectDests(); i < e; ++i) {
     MachineBasicBlock *Target = FuncInfo.MBBMap[I.getIndirectDest(i)];
     addSuccessorWithProb(CallBrMBB, Target, BranchProbability::getZero());

 }
 
 /// Handle PHI nodes in successor blocks.  Emit code into the SelectionDAG to
 /// ensure constants are generated when needed.  Remember the virtual registers
 /// that need to be added to the Machine PHI nodes as input.  We cannot just
 /// directly add them, because expansion might result in multiple MBB's for one
 /// BB.  As such, the start of the BB might correspond to a different MBB than
 /// the end.
-void
-SelectionDAGBuilder::HandlePHINodesInSuccessorBlocks(const BasicBlock *LLVMBB) {
+void SelectionDAGBuilder::HandlePHINodesInSuccessorBlocks(
+    const BasicBlock *LLVMBB, bool Preprocess) {
   const Instruction *TI = LLVMBB->getTerminator();
-
+  const CallBrInst *CI = dyn_cast<CallBrInst>(TI);
   SmallPtrSet<MachineBasicBlock *, 4> SuccsHandled;
 
   // Check PHI nodes in successors that expect a value to be available from this
   // block.
   for (unsigned succ = 0, e = TI->getNumSuccessors(); succ != e; ++succ) {
     const BasicBlock *SuccBB = TI->getSuccessor(succ);
     if (!isa<PHINode>(SuccBB->begin())) continue;
+
+    if (CI) {
+      if (Preprocess) {
+        // Don't push PHI node values back before an INLINEASM_BR instruction on
+        // the default branch.
+        if (SuccBB == CI->getDefaultDest())
+          continue;
+      } else {
+        // Don't push PHI node values back after an INLINEASM_BR instruction on
+        // the indirect branch.
+        if (SuccBB != CI->getDefaultDest())
+          continue;
+      }
+    }
+
     MachineBasicBlock *SuccMBB = FuncInfo.MBBMap[SuccBB];
 
     // If this terminator has multiple identical successors (common for
     // switches), only handle each succ once.
     if (!SuccsHandled.insert(SuccMBB).second)
       continue;
 
     MachineBasicBlock::iterator MBBI = SuccMBB->begin();
for (const PHINode &PN : SuccBB->phis()) {
         for (unsigned i = 0, e = NumRegisters; i != e; ++i)
           FuncInfo.PHINodesToUpdate.push_back(
               std::make_pair(&*MBBI++, Reg + i));
         Reg += NumRegisters;
       }
     }
   }
 
   ConstantsOut.clear();
 }
 
 /// Add a successor MBB to ParentMBB< creating a new MachineBB for BB if SuccMBB
 /// is 0.
 MachineBasicBlock *
 SelectionDAGBuilder::StackProtectorDescriptor::
 AddSuccessorMBB(const BasicBlock *BB,
                 MachineBasicBlock *ParentMBB,

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

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
+
+@var = global i32 0, align 4
+
+define i32 @test1() {
+; CHECK-LABEL: test1:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    jmp .Ltmp0
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:  # %bb.2:
+; CHECK-NEXT:    retl
+; CHECK-NEXT:  .Ltmp0: # Block address taken
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:    movl $1, %eax
+; CHECK-NEXT:    retl
+  callbr void asm sideeffect "jmp ${1:l}", "*m,X,~{dirflag},~{fpsr},~{flags}"(i32* nonnull @var, i8* blockaddress(@test1, %1))
+          to label %2 [label %1]
+
+1:                                                ; preds = %0
+  br label %2
+
+2:                                                ; preds = %0, %1
+  %3 = phi i32 [ 1, %1 ], [ 0, %0 ]
+  ret i32 %3
+}