From 7af9d386da2a2447ac044120ff23770ac0cedc3c Mon Sep 17 00:00:00 2001 From: James Y Knight Date: Thu, 20 Feb 2020 01:16:13 -0500 Subject: [PATCH] Correctly modify the CFG in IfConverter, and then remove the CorrectExtraCFGEdges function. The latter was a workaround for "Various pieces of code" leaving bogus extra CFG edges in place. Where by "various" it meant only IfConverter::MergeBlocks, which failed to clear all of the successors of dead blocks it emptied out. This wouldn't matter a whole lot, except that the dead blocks remained listed as predecessors of still-useful blocks, inhibiting optimizations. This fix slightly changed two thumb tests, because the correct CFG successors allowed for the "diamond" if-conversion pattern to be detected, when it could only use "simple" before. Additionally, the removal of a now-redundant call to analyzeBranch (with AllowModify=true) in BranchFolder::OptimizeFunction caused a later check for an empty block in BranchFolder::OptimizeBlock to fail. Correct this by moving the call to analyzeBranch in OptimizeBlock higher. Differential Revision: https://reviews.llvm.org/D79527 --- llvm/include/llvm/CodeGen/MachineBasicBlock.h | 10 --- llvm/lib/CodeGen/BranchFolding.cpp | 29 +++------ llvm/lib/CodeGen/IfConversion.cpp | 12 ++-- llvm/lib/CodeGen/MachineBasicBlock.cpp | 62 ------------------- llvm/test/CodeGen/ARM/ifcvt3.ll | 5 +- llvm/test/CodeGen/Thumb2/thumb2-ifcvt1.ll | 6 +- 6 files changed, 22 insertions(+), 102 deletions(-) diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h index 2788ef5c0103..3d486fa1e8d2 100644 --- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h +++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h @@ -837,16 +837,6 @@ public: /// instead of basic block \p Old. void replacePhiUsesWith(MachineBasicBlock *Old, MachineBasicBlock *New); - /// Various pieces of code can cause excess edges in the CFG to be inserted. - /// If we have proven that MBB can only branch to DestA and DestB, remove any - /// other MBB successors from the CFG. DestA and DestB can be null. Besides - /// DestA and DestB, retain other edges leading to LandingPads (currently - /// there can be only one; we don't check or require that here). Note it is - /// possible that DestA and/or DestB are LandingPads. - bool CorrectExtraCFGEdges(MachineBasicBlock *DestA, - MachineBasicBlock *DestB, - bool IsCond); - /// Find the next valid DebugLoc starting at MBBI, skipping any DBG_VALUE /// and DBG_LABEL instructions. Return UnknownLoc if there is none. DebugLoc findDebugLoc(instr_iterator MBBI); diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp index 7cf3eab2c06f..eb72542b6efe 100644 --- a/llvm/lib/CodeGen/BranchFolding.cpp +++ b/llvm/lib/CodeGen/BranchFolding.cpp @@ -202,14 +202,7 @@ bool BranchFolder::OptimizeFunction(MachineFunction &MF, if (!UpdateLiveIns) MRI.invalidateLiveness(); - // Fix CFG. The later algorithms expect it to be right. bool MadeChange = false; - for (MachineBasicBlock &MBB : MF) { - MachineBasicBlock *TBB = nullptr, *FBB = nullptr; - SmallVector Cond; - if (!TII->analyzeBranch(MBB, TBB, FBB, Cond, true)) - MadeChange |= MBB.CorrectExtraCFGEdges(TBB, FBB, !Cond.empty()); - } // Recalculate EH scope membership. EHScopeMembership = getEHScopeMembership(MF); @@ -1336,6 +1329,13 @@ ReoptimizeBlock: SameEHScope = MBBEHScope->second == FallThroughEHScope->second; } + // Analyze the branch in the current block. As a side-effect, this may cause + // the block to become empty. + MachineBasicBlock *CurTBB = nullptr, *CurFBB = nullptr; + SmallVector CurCond; + bool CurUnAnalyzable = + TII->analyzeBranch(*MBB, CurTBB, CurFBB, CurCond, true); + // If this block is empty, make everyone use its fall-through, not the block // explicitly. Landing pads should not do this since the landing-pad table // points to this block. Blocks with their addresses taken shouldn't be @@ -1378,10 +1378,6 @@ ReoptimizeBlock: bool PriorUnAnalyzable = TII->analyzeBranch(PrevBB, PriorTBB, PriorFBB, PriorCond, true); if (!PriorUnAnalyzable) { - // If the CFG for the prior block has extra edges, remove them. - MadeChange |= PrevBB.CorrectExtraCFGEdges(PriorTBB, PriorFBB, - !PriorCond.empty()); - // If the previous branch is conditional and both conditions go to the same // destination, remove the branch, replacing it with an unconditional one or // a fall-through. @@ -1549,15 +1545,7 @@ ReoptimizeBlock: } } - // Analyze the branch in the current block. - MachineBasicBlock *CurTBB = nullptr, *CurFBB = nullptr; - SmallVector CurCond; - bool CurUnAnalyzable = - TII->analyzeBranch(*MBB, CurTBB, CurFBB, CurCond, true); if (!CurUnAnalyzable) { - // If the CFG for the prior block has extra edges, remove them. - MadeChange |= MBB->CorrectExtraCFGEdges(CurTBB, CurFBB, !CurCond.empty()); - // If this is a two-way branch, and the FBB branches to this block, reverse // the condition so the single-basic-block loop is faster. Instead of: // Loop: xxx; jcc Out; jmp Loop @@ -1634,7 +1622,7 @@ ReoptimizeBlock: PMBB->ReplaceUsesOfBlockWith(MBB, CurTBB); // If this change resulted in PMBB ending in a conditional // branch where both conditions go to the same destination, - // change this to an unconditional branch (and fix the CFG). + // change this to an unconditional branch. MachineBasicBlock *NewCurTBB = nullptr, *NewCurFBB = nullptr; SmallVector NewCurCond; bool NewCurUnAnalyzable = TII->analyzeBranch( @@ -1646,7 +1634,6 @@ ReoptimizeBlock: TII->insertBranch(*PMBB, NewCurTBB, nullptr, NewCurCond, pdl); MadeChange = true; ++NumBranchOpts; - PMBB->CorrectExtraCFGEdges(NewCurTBB, nullptr, false); } } } diff --git a/llvm/lib/CodeGen/IfConversion.cpp b/llvm/lib/CodeGen/IfConversion.cpp index c332824f99f2..a3cace837eb9 100644 --- a/llvm/lib/CodeGen/IfConversion.cpp +++ b/llvm/lib/CodeGen/IfConversion.cpp @@ -2243,10 +2243,10 @@ void IfConverter::CopyAndPredicateBlock(BBInfo &ToBBI, BBInfo &FromBBI, } /// Move all instructions from FromBB to the end of ToBB. This will leave -/// FromBB as an empty block, so remove all of its successor edges except for -/// the fall-through edge. If AddEdges is true, i.e., when FromBBI's branch is -/// being moved, add those successor edges to ToBBI and remove the old edge -/// from ToBBI to FromBBI. +/// FromBB as an empty block, so remove all of its successor edges and move it +/// to the end of the function. If AddEdges is true, i.e., when FromBBI's +/// branch is being moved, add those successor edges to ToBBI and remove the old +/// edge from ToBBI to FromBBI. void IfConverter::MergeBlocks(BBInfo &ToBBI, BBInfo &FromBBI, bool AddEdges) { MachineBasicBlock &FromMBB = *FromBBI.BB; assert(!FromMBB.hasAddressTaken() && @@ -2286,8 +2286,10 @@ void IfConverter::MergeBlocks(BBInfo &ToBBI, BBInfo &FromBBI, bool AddEdges) { for (MachineBasicBlock *Succ : FromSuccs) { // Fallthrough edge can't be transferred. - if (Succ == FallThrough) + if (Succ == FallThrough) { + FromMBB.removeSuccessor(Succ); continue; + } auto NewProb = BranchProbability::getZero(); if (AddEdges) { diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp index 09e31afef26c..9abd24eccbb7 100644 --- a/llvm/lib/CodeGen/MachineBasicBlock.cpp +++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp @@ -1261,68 +1261,6 @@ void MachineBasicBlock::replacePhiUsesWith(MachineBasicBlock *Old, } } -/// Various pieces of code can cause excess edges in the CFG to be inserted. If -/// we have proven that MBB can only branch to DestA and DestB, remove any other -/// MBB successors from the CFG. DestA and DestB can be null. -/// -/// Besides DestA and DestB, retain other edges leading to LandingPads -/// (currently there can be only one; we don't check or require that here). -/// Note it is possible that DestA and/or DestB are LandingPads. -bool MachineBasicBlock::CorrectExtraCFGEdges(MachineBasicBlock *DestA, - MachineBasicBlock *DestB, - bool IsCond) { - // The values of DestA and DestB frequently come from a call to the - // 'TargetInstrInfo::analyzeBranch' method. We take our meaning of the initial - // values from there. - // - // 1. If both DestA and DestB are null, then the block ends with no branches - // (it falls through to its successor). - // 2. If DestA is set, DestB is null, and IsCond is false, then the block ends - // with only an unconditional branch. - // 3. If DestA is set, DestB is null, and IsCond is true, then the block ends - // with a conditional branch that falls through to a successor (DestB). - // 4. If DestA and DestB is set and IsCond is true, then the block ends with a - // conditional branch followed by an unconditional branch. DestA is the - // 'true' destination and DestB is the 'false' destination. - - bool Changed = false; - - MachineBasicBlock *FallThru = getNextNode(); - - if (!DestA && !DestB) { - // Block falls through to successor. - DestA = FallThru; - DestB = FallThru; - } else if (DestA && !DestB) { - if (IsCond) - // Block ends in conditional jump that falls through to successor. - DestB = FallThru; - } else { - assert(DestA && DestB && IsCond && - "CFG in a bad state. Cannot correct CFG edges"); - } - - // Remove superfluous edges. I.e., those which aren't destinations of this - // basic block, duplicate edges, or landing pads. - SmallPtrSet SeenMBBs; - MachineBasicBlock::succ_iterator SI = succ_begin(); - while (SI != succ_end()) { - const MachineBasicBlock *MBB = *SI; - if (!SeenMBBs.insert(MBB).second || - (MBB != DestA && MBB != DestB && !MBB->isEHPad())) { - // This is a superfluous edge, remove it. - SI = removeSuccessor(SI); - Changed = true; - } else { - ++SI; - } - } - - if (Changed) - normalizeSuccProbs(); - return Changed; -} - /// Find the next valid DebugLoc starting at MBBI, skipping any DBG_VALUE /// instructions. Return UnknownLoc if there is none. DebugLoc diff --git a/llvm/test/CodeGen/ARM/ifcvt3.ll b/llvm/test/CodeGen/ARM/ifcvt3.ll index e53d989ad529..be2a0fcc1d95 100644 --- a/llvm/test/CodeGen/ARM/ifcvt3.ll +++ b/llvm/test/CodeGen/ARM/ifcvt3.ll @@ -13,7 +13,9 @@ define i32 @t1(i32 %a, i32 %b, i32 %c, i32 %d) { cond_true: ; CHECK: addne r0 -; CHECK: bxne +; CHECK: addeq r0 +; CHECK: addeq r0 +; CHECK: bx %tmp12 = add i32 %a, 1 %tmp1518 = add i32 %tmp12, %b ret i32 %tmp1518 @@ -26,7 +28,6 @@ cond_next: ; CHECK-V4-CMP: cmpne ; CHECK-V4-CMP-NOT: cmpne -; CHECK-V4-BX: bx ; CHECK-V4-BX: bx ; CHECK-V4-BX-NOT: bx diff --git a/llvm/test/CodeGen/Thumb2/thumb2-ifcvt1.ll b/llvm/test/CodeGen/Thumb2/thumb2-ifcvt1.ll index a78b0c134263..1f69ac346340 100644 --- a/llvm/test/CodeGen/Thumb2/thumb2-ifcvt1.ll +++ b/llvm/test/CodeGen/Thumb2/thumb2-ifcvt1.ll @@ -4,10 +4,12 @@ ; RUN: llc < %s -mtriple=thumbv8 -arm-no-restrict-it -enable-tail-merge=0 | FileCheck %s define i32 @t1(i32 %a, i32 %b, i32 %c, i32 %d) nounwind { ; CHECK-LABEL: t1: -; CHECK: ittt ne +; CHECK: ittee ne ; CHECK: cmpne ; CHECK: addne -; CHECK: bxne lr +; CHECK: addeq +; CHECK: addeq +; CHECK: bx lr switch i32 %c, label %cond_next [ i32 1, label %cond_true i32 7, label %cond_true