[SelectionDAG] remove stale check assuming INLINEASM_BR is terminator

[nickdesaulniers]

Summary

D79794 changed INLINEASM_BR from being a termintor to not.

While reading though SelectionDAGBuilder::visitInlineAsm, I noticed a
comment regarding INLINEASM_BR being a terminator, which seemed stale.

Removing the code that has an assumption does change the placement of a
TokenFactor in one test, but does not change the final output. The
assembler generated is ultimately the same.

[nickdesaulniers]

See also:

commit 97d4f7c19414 ("[SelectionDAGBuilder] Flush PendingExports before creating INLINEASM_BR node for asm goto.")

D59981

[nickdesaulniers]

Also, commit e97e52757cfb ("[X86] Add test case for r361177.")

https://reviews.llvm.org/rGe97e52757cfb

[void]

For the love of God, that test output is unreadable...

This LGTM, but @craig.topper should check to make sure.

[craig.topper]

llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll

22

This means the copies to virtual register %0 and %1 are no longer guaranteed to happen before the inlineasm_br block is executed. Since I think one of those is used by the fail basic block, that seems wrong.

[nickdesaulniers]

@craig.topper mentions on discord:

A TokenFactor collects multiple chains into a single chain. Chains are used to maintain order of things that don't have a natural data dependency.
Previously the inlineasm_br chain was dependent on the copy chains so it had to occur after them. Now they are considered independent.

Perhaps then the best way forward here is to simply adapt the existing comment that is still stale? Somehow?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitInlineAsm(const CallBase &Call,
   // We won't need to flush pending loads if this asm doesn't touch
   // memory and is nonvolatile.
   SDValue Flag, Chain = (HasSideEffect) ? getRoot() : DAG.getRoot();
 
   bool EmitEHLabels = isa<InvokeInst>(Call) && IA->canThrow();
   if (EmitEHLabels) {
     assert(EHPadBB && "InvokeInst must have an EHPadBB");
   }
-  bool IsCallBr = isa<CallBrInst>(Call);
 
-  if (IsCallBr || EmitEHLabels) {
-    // If this is a callbr or invoke we need to flush pending exports since
-    // inlineasm_br and invoke are terminators.
-    // We need to do this before nodes are glued to the inlineasm_br node.
+  // If this is an invoke we need to flush pending exports since it is a
+  // terminator.
+  if (EmitEHLabels)
     Chain = getControlRoot();
-  }
 
   MCSymbol *BeginLabel = nullptr;
   if (EmitEHLabels) {
     Chain = lowerStartEH(Chain, EHPadBB, BeginLabel);
   }
 
   int OpNo = -1;
   SmallVector<StringRef> AsmStrs;
case InlineAsm::isClobber:
       break;
     }
   }
 
   // Finish up input operands.  Set the input chain and add the flag last.
   AsmNodeOperands[InlineAsm::Op_InputChain] = Chain;
   if (Flag.getNode()) AsmNodeOperands.push_back(Flag);
 
+  const bool IsCallBr = isa<CallBrInst>(Call);
   unsigned ISDOpc = IsCallBr ? ISD::INLINEASM_BR : ISD::INLINEASM;
   Chain = DAG.getNode(ISDOpc, getCurSDLoc(),
                       DAG.getVTList(MVT::Other, MVT::Glue), AsmNodeOperands);
   Flag = Chain.getValue(1);
 
   // Do additional work to generate outputs.
 
   SmallVector<EVT, 1> ResultVTs;

llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll

 ; REQUIRES: asserts
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -o /dev/null -debug-only=isel 2>&1 | FileCheck %s
 
-; Make sure we emit the basic block exports and the TokenFactor before the
-; inlineasm_br. Not sure how to get a MachineIR change so this reads the debug
+; Make sure the TokenFactor depends on the basic block exports and the
+; INLINEASM_BR.
+; Not sure how to get a MachineIR change so this reads the debug
 ; output from SelectionDAG.
 
-; CHECK: t0: ch,glue = EntryToken
+; CHECK: Optimized legalized selection DAG: %bb.0 'test:entry'
+; CHECK-NEXT: SelectionDAG has 32 nodes:
+; CHECK-NEXT: t0: ch,glue = EntryToken
+; CHECK-NEXT: t2: i32,ch = CopyFromReg t0, Register:i32 %2
+; CHECK-NEXT: t8: i32 = add t2, Constant:i32<4>
+; CHECK-NEXT: t21: ch,glue = CopyToReg t0, Register:i32 %5, t8
 ; CHECK-NEXT: t4: i32,ch = CopyFromReg t0, Register:i32 %3
 ; CHECK-NEXT: t10: i32 = add t4, Constant:i32<1>
 ; CHECK-NEXT: t12: ch = CopyToReg t0, Register:i32 %0, t10
 ; CHECK-NEXT: t6: i32,ch = CopyFromReg t0, Register:i32 %4
 ; CHECK-NEXT: t13: i32 = add t6, Constant:i32<1>
 ; CHECK-NEXT: t15: ch = CopyToReg t0, Register:i32 %1, t13
-; CHECK-NEXT: t17: ch = TokenFactor t12, t15
-; CHECK-NEXT: t2: i32,ch = CopyFromReg t0, Register:i32 %2
-; CHECK-NEXT: t8: i32 = add t2, Constant:i32<4>
-; CHECK-NEXT: t22: ch,glue = CopyToReg t17, Register:i32 %5, t8
-; CHECK-NEXT: t29: ch,glue = inlineasm_br t22, TargetExternalSymbol:i64'xorl $0, $0; jmp ${1:l}', MDNode:ch<null>, TargetConstant:i64<0>, TargetConstant:i32<2359305>, Register:i32 %5, TargetConstant:i64<13>, BasicBlock:ch<fail 0x{{[0-9a-f]+}}>, TargetConstant:i32<12>, Register:i32 $df, TargetConstant:i32<12>, Register:i16 $fpsw, TargetConstant:i32<12>, Register:i32 $eflags, t22:1
+; CHECK-NEXT: t28: ch,glue = inlineasm_br t21, TargetExternalSymbol:i64'xorl $0, $0; jmp ${1:l}', MDNode:ch<null>, TargetConstant:i64<0>, TargetConstant:i32<2359305>, Register:i32 %5, TargetConstant:i64<13>, BasicBlock:ch<fail 0x{{[0-9a-f]+}}>, TargetConstant:i32<12>, Register:i32 $df, TargetConstant:i32<12>, Register:i16 $fpsw, TargetConstant:i32<12>, Register:i32 $eflags, t21:1
+; CHECK-NEXT: t29: ch = TokenFactor t12, t15, t28
+; CHECK-NEXT: t31: ch = br t29, BasicBlock:ch<normal 0x{{[0-9a-f]+}}>
+
 
 define i32 @test(i32 %a, i32 %b, i32 %c) {
 entry:
   %0 = add i32 %a, 4
   %1 = add i32 %b, 1
   %2 = add i32 %c, 1
   callbr void asm "xorl $0, $0; jmp ${1:l}", "r,!i,~{dirflag},~{fpsr},~{flags}"(i32 %0) to label %normal [label %fail]
 
 normal:
   ret i32 %1
 
 fail:
   ret i32 %2
 }