FastISel: Avoid adding a successor block twice for degenerate IR.

[mcrosier]

Matthias,
Juergen is probably more familiar with FastISel than I these days, but the patch itself looks reasonable. I'll let Juergen give the final LGTM, however.

Is the degenerate IR a result of not running the optimizers (i.e., using -O0)? I'm just trying to understand how we arrived at this situation to determine if this really is the correct place to address the problem.

Chad

[probinson]

I should think that if the input IR is legal, LLVM must not crash. Therefore, ultimately this defensive patch seems appropriate.

But this patch is in response to PR24581, which is about a function with 'optnone' attribute. The goal of optnone is to approximate -O0 even if you didn't say -O0. If optnone is running something it should not, or failing to run something it really should, it's worth knowing. The set of transformations that 'optnone' turns off was determined in a very ad-hoc way and it's *very* possible that there's a missing or extra check somewhere. I'd like to see some kind of description of how the IR got into this state (or failed to get out of it).

[MatzeB]

Yes this code is the result of an optnone attribute. But in any case this is legal IR and SelectionDAG handles this case in the same way[1], so there is no reason for FastISel to produce invalid MI.

[1] SelectionDAGBuilder.cpp:

// Update successor info
addSuccessorWithWeight(SwitchBB, CB.TrueBB, CB.TrueWeight);
// TrueBB and FalseBB are always different unless the incoming IR is
// degenerate. This only happens when running llc on weird IR.
if (CB.TrueBB != CB.FalseBB)
  addSuccessorWithWeight(SwitchBB, CB.FalseBB, CB.FalseWeight);

[mcrosier]

LGTM.

[mcrosier]

In D12350#233249, @mcrosier wrote:

LGTM.

Obviously, ignore my first comment about deferring to Juergen.. :) This seems quite reasonable.

llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp

 
 void FastISel::finishCondBranch(const BasicBlock *BranchBB,
                                 MachineBasicBlock *TrueMBB,
                                 MachineBasicBlock *FalseMBB) {
   uint32_t BranchWeight = 0;
   if (FuncInfo.BPI)
     BranchWeight = FuncInfo.BPI->getEdgeWeight(BranchBB,
                                                TrueMBB->getBasicBlock());
+  // Add TrueMBB as successor unless it is equal to the FalseMBB: This can
+  // happen in degenerate IR and MachineIR forbids to have a block twice in the
+  // successor/predecessor lists.
+  if (TrueMBB != FalseMBB)
     FuncInfo.MBB->addSuccessor(TrueMBB, BranchWeight);
 
   fastEmitBranch(FalseMBB, DbgLoc);
 }
 
 /// Emit an FNeg operation.
 bool FastISel::selectFNeg(const User *I) {
   unsigned OpReg = getRegForValue(BinaryOperator::getFNegArgument(I));
   if (!OpReg)

llvm/trunk/test/CodeGen/X86/fast-isel-cmp-branch.ll

-; RUN: llc -O0 -mtriple=x86_64-linux -asm-verbose=false < %s | FileCheck %s
-; RUN: llc -O0 -mtriple=x86_64-windows-itanium -asm-verbose=false < %s | FileCheck %s
+; RUN: llc -O0 -mtriple=x86_64-linux -asm-verbose=false -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -O0 -mtriple=x86_64-windows-itanium -asm-verbose=false -verify-machineinstrs < %s | FileCheck %s
+
+; Fast-isel mustn't add a block to the MBB successor/predecessor list twice.
+; The machine verifier will catch and complain about this case.
+; CHECK-LABEL: baz
+; CHECK: retq
+define void @baz() {
+entry:
+  br i1 undef, label %exit, label %exit
+
+exit:
+  ret void
+}
+
 ; rdar://8337108
 
 ; Fast-isel shouldn't try to look through the compare because it's in a
 ; different basic block, so its operands aren't necessarily exported
 ; for cross-block usage.
 
 ; CHECK: movb    %al, [[OFS:[0-9]*]](%rsp)
 ; CHECK: callq   {{_?}}bar