[CodeGen] Make branch funnels pass the machine verifier

[thegameg]

Summary

We previously marked all the tests with branch funnels as -verify-machineinstrs=0.

This is an attempt to fix it.

1. ICALL_BRANCH_FUNNEL has no defs. Mark it as let OutOperandList = (outs)

2. After that we hit an assert:

Assertion failed: (Op.getValueType() != MVT::Other && Op.getValueType() != MVT::Glue && "Chain and glue operands should occur at end of operand list!"), function AddOperand, file /Users/francisvm/llvm/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp,
line 461.

The chain operand was added at the beginning of the operand list. Move that to the end.

3. After that we hit another verifier issue in the pseudo expansion where the registers used in the cmps and jmps are not added to the livein lists. Add the EFLAGS to all the new MBBs that we create.

PR39436

[thegameg]

Ping?

[thegameg]

Ping?

[thegameg]

Ping

[chandlerc]

We found that branch funnels performed worse than retpolines after PGO and devirtualization, maybe we should just remove branch funnels? what do you think Peter?

[tejohnson]

@pcc can you take a look? The same failure just popped up in the new devirt test I added today (test added in r364960, workaround added in r364997).

[pcc]

LGTM

[thegameg]

Landed in r365058.

include/llvm/Target/Target.td

def FENTRY_CALL : StandardPseudoInstruction {
   let InOperandList = (ins);
   let AsmString = "# FEntry call";
   let usesCustomInserter = 1;
   let mayLoad = 1;
   let mayStore = 1;
   let hasSideEffects = 1;
 }
 def ICALL_BRANCH_FUNNEL : StandardPseudoInstruction {
-  let OutOperandList = (outs unknown:$dst);
+  let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "";
   let hasSideEffects = 1;
 }
 
 // Generic opcodes used in GlobalISel.
 include "llvm/Target/GenericOpcodes.td"
 

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

   case Intrinsic::experimental_vector_reduce_umin:
   case Intrinsic::experimental_vector_reduce_fmax:
   case Intrinsic::experimental_vector_reduce_fmin:
     visitVectorReduce(I, Intrinsic);
     return nullptr;
 
   case Intrinsic::icall_branch_funnel: {
     SmallVector<SDValue, 16> Ops;
-    Ops.push_back(DAG.getRoot());
     Ops.push_back(getValue(I.getArgOperand(0)));
 
     int64_t Offset;
     auto *Base = dyn_cast<GlobalObject>(GetPointerBaseWithConstantOffset(
         I.getArgOperand(1), Offset, DAG.getDataLayout()));
     if (!Base)
       report_fatal_error(
           "llvm.icall.branch.funnel operand must be a GlobalValue");
llvm::sort(Targets,
                  return T1.Offset < T2.Offset;
                });
 
     for (auto &T : Targets) {
       Ops.push_back(DAG.getTargetConstant(T.Offset, getCurSDLoc(), MVT::i32));
       Ops.push_back(T.Target);
     }
 
+    Ops.push_back(DAG.getRoot()); // Chain
+
     SDValue N(DAG.getMachineNode(TargetOpcode::ICALL_BRANCH_FUNNEL,
                                  getCurSDLoc(), MVT::Other, Ops),
               0);
     DAG.setRoot(N);
     setValue(&I, N);
     HasTailCall = true;
     return nullptr;
   }

lib/Target/X86/X86ExpandPseudo.cpp

void X86ExpandPseudo::ExpandICallBranchFunnel(
   ++InsPt;
 
   std::vector<std::pair<MachineBasicBlock *, unsigned>> TargetMBBs;
   DebugLoc DL = JTInst->getDebugLoc();
   MachineOperand Selector = JTInst->getOperand(0);
   const GlobalValue *CombinedGlobal = JTInst->getOperand(1).getGlobal();
 
   auto CmpTarget = [&](unsigned Target) {
+    if (Selector.isReg())
+      MBB->addLiveIn(Selector.getReg());
     BuildMI(*MBB, MBBI, DL, TII->get(X86::LEA64r), X86::R11)
         .addReg(X86::RIP)
         .addImm(1)
         .addReg(0)
         .addGlobalAddress(CombinedGlobal,
                           JTInst->getOperand(2 + 2 * Target).getImm())
         .addReg(0);
     BuildMI(*MBB, MBBI, DL, TII->get(X86::CMP64rr))
         .add(Selector)
         .addReg(X86::R11);
   };
 
   auto CreateMBB = [&]() {
     auto *NewMBB = MF->CreateMachineBasicBlock(BB);
     MBB->addSuccessor(NewMBB);
+    if (!MBB->isLiveIn(X86::EFLAGS))
+      MBB->addLiveIn(X86::EFLAGS);
     return NewMBB;
   };
 
   auto EmitCondJump = [&](unsigned Opcode, MachineBasicBlock *ThenMBB) {
     BuildMI(*MBB, MBBI, DL, TII->get(Opcode)).addMBB(ThenMBB);
 
     auto *ElseMBB = CreateMBB();
     MF->insert(InsPt, ElseMBB);

test/CodeGen/X86/icall-branch-funnel.ll

-; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
-; RUN: llc -mtriple=x86_64-unknown-linux -verify-machineinstrs=0 < %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-unknown-linux < %s | FileCheck %s
 
 @g = external global i8
 
 declare void @f0()
 declare void @f1()
 declare void @f2()
 declare void @f3()
 declare void @f4()

test/ThinLTO/X86/cfi-devirt.ll

 ; REQUIRES: x86-registered-target
 
 ; Test CFI devirtualization through the thin link and backend.
 
 ; RUN: opt -thinlto-bc -o %t.o %s
 
 ; Legacy PM
-; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
 ; RUN: llvm-lto2 run %t.o -save-temps -pass-remarks=. \
-; RUN:   -verify-machineinstrs=0 \
 ; RUN:   -o %t3 \
 ; RUN:   -r=%t.o,test,px \
 ; RUN:   -r=%t.o,_ZN1A1nEi,p \
 ; RUN:   -r=%t.o,_ZN1B1fEi,p \
 ; RUN:   -r=%t.o,_ZN1C1fEi,p \
 ; RUN:   -r=%t.o,empty,p \
 ; RUN:   -r=%t.o,_ZTV1B, \
 ; RUN:   -r=%t.o,_ZTV1C, \
 ; RUN:   -r=%t.o,_ZN1A1nEi, \
 ; RUN:   -r=%t.o,_ZN1B1fEi, \
 ; RUN:   -r=%t.o,_ZN1C1fEi, \
 ; RUN:   -r=%t.o,_ZTV1B,px \
 ; RUN:   -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
 ; New PM
-; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
 ; RUN: llvm-lto2 run %t.o -save-temps -use-new-pm -pass-remarks=. \
-; RUN:   -verify-machineinstrs=0 \
 ; RUN:   -o %t3 \
 ; RUN:   -r=%t.o,test,px \
 ; RUN:   -r=%t.o,_ZN1A1nEi,p \
 ; RUN:   -r=%t.o,_ZN1B1fEi,p \
 ; RUN:   -r=%t.o,_ZN1C1fEi,p \
 ; RUN:   -r=%t.o,empty,p \
 ; RUN:   -r=%t.o,_ZTV1B, \
 ; RUN:   -r=%t.o,_ZTV1C, \

test/ThinLTO/X86/devirt-after-icp.ll

 ; which will allow us to devirtualize indirect call ((B*)a)->bar() to B::bar();
 ; Both that type test and the one for the fallback a->foo() indirect call
 ; will use the same vtable pointer. Without a dominance check, we could
 ; incorrectly devirtualize a->foo() to B::foo();
 
 ; RUN: opt -thinlto-bc -o %t.o %s
 
 ; Legacy PM
-; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
 ; RUN: llvm-lto2 run %t.o -save-temps -pass-remarks=. \
-; RUN:   -verify-machineinstrs=0 \
 ; RUN:   -o %t3 \
 ; RUN:   -r=%t.o,_Z3bazP1A,px \
 ; RUN:   -r=%t.o,_ZN1A3fooEv, \
 ; RUN:   -r=%t.o,_ZN1A3barEv, \
 ; RUN:   -r=%t.o,_ZN1B3fooEv, \
 ; RUN:   -r=%t.o,_ZN1B3barEv, \
 ; RUN:   -r=%t.o,_ZTV1A, \
 ; RUN:   -r=%t.o,_ZTV1B, \
 ; RUN:   -r=%t.o,_ZN1A3fooEv, \
 ; RUN:   -r=%t.o,_ZN1A3barEv, \
 ; RUN:   -r=%t.o,_ZN1B3fooEv, \
 ; RUN:   -r=%t.o,_ZN1B3barEv, \
 ; RUN:   -r=%t.o,_ZTV1A,px \
 ; RUN:   -r=%t.o,_ZTV1B,px 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
 ; New PM
-; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
 ; RUN: llvm-lto2 run %t.o -save-temps -use-new-pm -pass-remarks=. \
-; RUN:   -verify-machineinstrs=0 \
 ; RUN:   -o %t3 \
 ; RUN:   -r=%t.o,_Z3bazP1A,px \
 ; RUN:   -r=%t.o,_ZN1A3fooEv, \
 ; RUN:   -r=%t.o,_ZN1A3barEv, \
 ; RUN:   -r=%t.o,_ZN1B3fooEv, \
 ; RUN:   -r=%t.o,_ZN1B3barEv, \
 ; RUN:   -r=%t.o,_ZTV1A, \
 ; RUN:   -r=%t.o,_ZTV1B, \