[X86][NFCI] Remove target-specific branch optimisation that's handled in BranchFolding

[asb]

Summary

I was looking at other target's implementations of analyzeBranch to see if there's anything we're neglecting to handle for RISC-V. It looks like this optimisation in analyzeBranch is handled in OptimizeBlock in BranchFolding, so it's not clear there's any advantage in repeating in in X86's analyzeBranch. Certainly, no tests fail upon removing it.

Perhaps there's a benefit of handling it here that I'm missing?

[asb]

Ping.

[RKSimon]

LGTM but I don't know this code very well - @pengfei @craig.topper any comments?

[pengfei]

There seems a test for it: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/brcond.ll#L39
Maybe it has become dead code. LGTM.

[RKSimon]

In D129204#3668746, @pengfei wrote:

There seems a test for it: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/brcond.ll#L39
Maybe it has become dead code. LGTM.

@asb please can you confirm if the tests fail if the OptimizeBlock variant of the code is disabled as well?

[asb]

In D129204#3668750, @RKSimon wrote:

In D129204#3668746, @pengfei wrote:

There seems a test for it: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/brcond.ll#L39
Maybe it has become dead code. LGTM.

@asb please can you confirm if the tests fail if the OptimizeBlock variant of the code is disabled as well?

Weirdly that particular test in brcond.ll doesn't fail, but if disabling the code that implements this transform in OptimizeBlock:

--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -1452,17 +1452,17 @@ ReoptimizeBlock:
     // If the prior block branches here on true and somewhere else on false, and
     // if the branch condition is reversible, reverse the branch to create a
     // fall-through.
-    if (PriorTBB == MBB) {
-      SmallVector<MachineOperand, 4> NewPriorCond(PriorCond);
-      if (!TII->reverseBranchCondition(NewPriorCond)) {
-        DebugLoc dl = getBranchDebugLoc(PrevBB);
-        TII->removeBranch(PrevBB);
-        TII->insertBranch(PrevBB, PriorFBB, nullptr, NewPriorCond, dl);
-        MadeChange = true;
-        ++NumBranchOpts;
-        goto ReoptimizeBlock;
-      }
-    }
+//  if (PriorTBB == MBB) {
+//    SmallVector<MachineOperand, 4> NewPriorCond(PriorCond);
+//    if (!TII->reverseBranchCondition(NewPriorCond)) {
+//      DebugLoc dl = getBranchDebugLoc(PrevBB);
+//      TII->removeBranch(PrevBB);
+//      TII->insertBranch(PrevBB, PriorFBB, nullptr, NewPriorCond, dl);
+//      MadeChange = true;
+//      ++NumBranchOpts;
+//      goto ReoptimizeBlock;
+//    }
+//  }

Then the following X86 tests fail:

Failed Tests (8):
  LLVM :: CodeGen/X86/2010-09-17-SideEffectsInChain.ll
  LLVM :: CodeGen/X86/atomic-flags.ll
  LLVM :: CodeGen/X86/fp-une-cmp.ll
  LLVM :: CodeGen/X86/ins_subreg_coalesce-3.ll
  LLVM :: CodeGen/X86/reverse_branches.ll
  LLVM :: CodeGen/X86/speculative-load-hardening.ll
  LLVM :: CodeGen/X86/stack-protector-3.ll
  LLVM :: CodeGen/X86/switch.ll

[RKSimon]

LGTM - thanks for checking - I can't see any reason we duplicate this

llvm/lib/Target/X86/X86InstrInfo.cpp

while (I != MBB.begin()) {
 
     // In practice we should never have an undef eflags operand, if we do
     // abort here as we are not prepared to preserve the flag.
     if (I->findRegisterUseOperand(X86::EFLAGS)->isUndef())
       return true;
 
     // Working from the bottom, handle the first conditional branch.
     if (Cond.empty()) {
-      MachineBasicBlock *TargetBB = I->getOperand(0).getMBB();
-      if (AllowModify && UnCondBrIter != MBB.end() &&
-          MBB.isLayoutSuccessor(TargetBB)) {
-        // If we can modify the code and it ends in something like:
-        //
-        //     jCC L1
-        //     jmp L2
-        //   L1:
-        //     ...
-        //   L2:
-        //
-        // Then we can change this to:
-        //
-        //     jnCC L2
-        //   L1:
-        //     ...
-        //   L2:
-        //
-        // Which is a bit more efficient.
-        // We conditionally jump to the fall-through block.
-        BranchCode = GetOppositeBranchCondition(BranchCode);
-        MachineBasicBlock::iterator OldInst = I;
-
-        BuildMI(MBB, UnCondBrIter, MBB.findDebugLoc(I), get(X86::JCC_1))
-          .addMBB(UnCondBrIter->getOperand(0).getMBB())
-          .addImm(BranchCode);
-        BuildMI(MBB, UnCondBrIter, MBB.findDebugLoc(I), get(X86::JMP_1))
-          .addMBB(TargetBB);
-
-        OldInst->eraseFromParent();
-        UnCondBrIter->eraseFromParent();
-
-        // Restart the analysis.
-        UnCondBrIter = MBB.end();
-        I = MBB.end();
-        continue;
-      }
-
       FBB = TBB;
       TBB = I->getOperand(0).getMBB();
       Cond.push_back(MachineOperand::CreateImm(BranchCode));
       CondBranches.push_back(&*I);
       continue;
     }
 
     // Handle subsequent conditional branches. Only handle the case where all