[llvm] r271925 - [MBP] Reduce code size by running tail merging in MBP.
via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 7 22:44:29 PDT 2016
Hi Diana,
I think I figure out what happens, but I am not sure if it is my bug or
a Thumb bug or something else.
My patch tries to run Tail Merging after Block Placement to reduce code
size. In the failing case, I have following four consecutive MBBs after
the Block Placement
BB#187: derived from LLVM BB %if.then.i.i.i673
t2Bcc <BB#193>, pred:3, pred:%CPSR<kill>
t2B <BB#188>, pred:14, pred:%noreg
BB#190: derived from LLVM BB %if.then9.i.i.i
t2Bcc <BB#188>, pred:2, pred:%CPSR<kill>
BB#193: derived from LLVM BB %if.then2.i
...
BB#188: derived from LLVM BB %if.end5.i
...
BB#187 and BB#190 are regarded the same (both only contain branches that
have same conditions and same targets) and can be merged. Tail Merging
calls ReplaceTailWithBranchTo() to replace the two branches in BB#187
with an unconditional branch to BB#190. It seems only Thumb2 overwrites
ReplaceTailWithBranchTo() and all the other targets just use
TargetInstrInfo::ReplaceTailWithBranchTo().
The crash comes from the following five lines of
Thumb2InstrInfo::ReplaceTailWithBranchTo()
// If the first instruction of Tail is predicated, we may have to
update
// the IT instruction.
unsigned PredReg = 0;
ARMCC::CondCodes CC = getInstrPredicate(*Tail, PredReg);
MachineBasicBlock::iterator MBBI = Tail;
if (CC != ARMCC::AL)
// Expecting at least the t2IT instruction before it.
--MBBI;
Variable Tail points to the first instruction of BB#187. --MBBI then
triggers the assertion.
I am not an expert of Thumb2 backend. Is it a bug of the implementation
of ReplaceTailWithBranchTo()? Or BB#187 is mistakenly treated as an IT
block (no t2IT instruction)? Or I should not call tail merging after
Block Placement for Thumb2 at all?
I think Thumb2 cares the code size and my patch is beneficial to Thumb2.
Please let me know what you think.
Thank you,
Haicheng
-----Original Message-----
From: Diana Picus [mailto:diana.picus at linaro.org]
Sent: Tuesday, June 07, 2016 12:11 PM
To: Haicheng Wu <haicheng at codeaurora.org>
Cc: llvm-commits <llvm-commits at lists.llvm.org>; Renato Golin
<renato.golin at linaro.org>
Subject: Re: [llvm] r271925 - [MBP] Reduce code size by running tail
merging
in MBP.
On 7 June 2016 at 18:24, Haicheng Wu <haicheng at codeaurora.org> wrote:
> Hi Diana,
>
> Sorry for this. I revert the patch in r272017 . If possible, would
> you
> please send me a reduced test case? Thank you.
Thanks, I'm currently reducing it on a machine similar to the bot, but
it
might take a while. I'll get back to you when it's done.
>
> Best,
>
> Haicheng
>
> -----Original Message-----
> From: Diana Picus [mailto:diana.picus at linaro.org]
> Sent: Tuesday, June 07, 2016 10:00 AM
> To: Haicheng Wu <haicheng at codeaurora.org>
> Cc: llvm-commits <llvm-commits at lists.llvm.org>; Renato Golin
> <renato.golin at linaro.org>
> Subject: Re: [llvm] r271925 - [MBP] Reduce code size by running tail
> merging in MBP.
>
> Hi,
>
> We've done some investigations and it seems this commit broke our thumb
> selfhosting bot:
> http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/buil
> ds/3602
>
> For some reason, we can't revert it. Can you help us fix this?
>
> I've attached the source file + script to reproduce the issue. I can
> work
> on a smaller reproducer if that helps.
>
> Thanks,
> Diana
>
> On 6 June 2016 at 21:36, Haicheng Wu via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>> Author: haicheng
>> Date: Mon Jun 6 13:36:07 2016
>> New Revision: 271925
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=271925&view=rev
>> Log:
>> [MBP] Reduce code size by running tail merging in MBP.
>>
>> The code layout that TailMerging (inside BranchFolding) works on is
>> not the final layout optimized based on the branch probability.
>> Generally, after BlockPlacement, many new merging opportunities
>> emerge.
>>
>> This patch calls Tail Merging after MBP and calls MBP again if Tail
>> Merging merges anything.
>>
>> Differential Revision: http://reviews.llvm.org/D20276
>>
>> Added:
>> llvm/trunk/test/CodeGen/AArch64/tailmerging_in_mbp.ll
>> Modified:
>> llvm/trunk/lib/CodeGen/BranchFolding.cpp
>> llvm/trunk/lib/CodeGen/BranchFolding.h
>> llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
>> llvm/trunk/test/CodeGen/ARM/arm-and-tst-peephole.ll
>>
>> Modified: llvm/trunk/lib/CodeGen/BranchFolding.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/BranchFold
>> i ng.cpp?rev=271925&r1=271924&r2=271925&view=diff
>> =====================================================================
>> =
>> ========
>> --- llvm/trunk/lib/CodeGen/BranchFolding.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/BranchFolding.cpp Mon Jun 6 13:36:07 2016
>> @@ -27,6 +27,7 @@
>> #include "llvm/CodeGen/MachineFunctionPass.h"
>> #include "llvm/CodeGen/MachineJumpTableInfo.h"
>> #include "llvm/CodeGen/MachineMemOperand.h"
>> +#include "llvm/CodeGen/MachineLoopInfo.h"
>> #include "llvm/CodeGen/MachineModuleInfo.h"
>> #include "llvm/CodeGen/MachineRegisterInfo.h"
>> #include "llvm/CodeGen/Passes.h"
>> @@ -137,6 +138,8 @@ void BranchFolder::RemoveDeadBlock(Machi
>> // Remove the block.
>> MF->erase(MBB);
>> FuncletMembership.erase(MBB);
>> + if (MLI)
>> + MLI->removeBlock(MBB);
>> }
>>
>> /// OptimizeImpDefsBlock - If a basic block is just a bunch of
>> implicit_def @@ -193,18 +196,22 @@ bool
>> BranchFolder::OptimizeImpDefsBlock(
>> }
>>
>> /// OptimizeFunction - Perhaps branch folding, tail merging and
>> other -/// CFG optimizations on the given function.
>> +/// CFG optimizations on the given function. Block placement
>> +changes the layout /// and may create new tail merging opportunities.
>> bool BranchFolder::OptimizeFunction(MachineFunction &MF,
>> const TargetInstrInfo *tii,
>> const TargetRegisterInfo *tri,
>> - MachineModuleInfo *mmi) {
>> + MachineModuleInfo *mmi,
>> + MachineLoopInfo *mli, bool
>> + AfterPlacement) {
>> if (!tii) return false;
>>
>> TriedMerging.clear();
>>
>> + AfterBlockPlacement = AfterPlacement;
>> TII = tii;
>> TRI = tri;
>> MMI = mmi;
>> + MLI = mli;
>> RS = nullptr;
>>
>> // Use a RegScavenger to help update liveness when required.
>> @@ -230,7 +237,10 @@ bool BranchFolder::OptimizeFunction(Mach
>> bool MadeChangeThisIteration = true;
>> while (MadeChangeThisIteration) {
>> MadeChangeThisIteration = TailMergeBlocks(MF);
>> - MadeChangeThisIteration |= OptimizeBranches(MF);
>> + // No need to clean up if tail merging does not change anything
>> after the
>> + // block placement.
>> + if (!AfterBlockPlacement || MadeChangeThisIteration)
>> + MadeChangeThisIteration |= OptimizeBranches(MF);
>> if (EnableHoistCommonCode)
>> MadeChangeThisIteration |= HoistCommonCode(MF);
>> MadeChange |= MadeChangeThisIteration; @@ -447,6 +457,11 @@
>> MachineBasicBlock *BranchFolder::SplitMB
>> // Splice the code over.
>> NewMBB->splice(NewMBB->end(), &CurMBB, BBI1, CurMBB.end());
>>
>> + // NewMBB belongs to the same loop as CurMBB.
>> + if (MLI)
>> + if (MachineLoop *ML = MLI->getLoopFor(&CurMBB))
>> + ML->addBasicBlockToLoop(NewMBB, MLI->getBase());
>> +
>> // NewMBB inherits CurMBB's block frequency.
>> MBBFreqInfo.setBlockFreq(NewMBB,
>> MBBFreqInfo.getBlockFreq(&CurMBB));
>>
>> @@ -934,23 +949,27 @@ bool BranchFolder::TailMergeBlocks(Machi
>> if (!EnableTailMerge) return MadeChange;
>>
>> // First find blocks with no successors.
>> - MergePotentials.clear();
>> - for (MachineBasicBlock &MBB : MF) {
>> + // Block placement does not create new tail merging opportunities
>> + for these // blocks.
>> + if (!AfterBlockPlacement) {
>> + MergePotentials.clear();
>> + for (MachineBasicBlock &MBB : MF) {
>> + if (MergePotentials.size() == TailMergeThreshold)
>> + break;
>> + if (!TriedMerging.count(&MBB) && MBB.succ_empty())
>> +
>> MergePotentials.push_back(MergePotentialsElt(HashEndOfMBB(MBB),
>> &MBB));
>> + }
>> +
>> + // If this is a large problem, avoid visiting the same basic
>> blocks
>> + // multiple times.
>> if (MergePotentials.size() == TailMergeThreshold)
>> - break;
>> - if (!TriedMerging.count(&MBB) && MBB.succ_empty())
>> - MergePotentials.push_back(MergePotentialsElt(HashEndOfMBB(MBB),
>> &MBB));
>> - }
>> + for (unsigned i = 0, e = MergePotentials.size(); i != e; ++i)
>> + TriedMerging.insert(MergePotentials[i].getBlock());
>>
>> - // If this is a large problem, avoid visiting the same basic
>> blocks
>> - // multiple times.
>> - if (MergePotentials.size() == TailMergeThreshold)
>> - for (unsigned i = 0, e = MergePotentials.size(); i != e; ++i)
>> - TriedMerging.insert(MergePotentials[i].getBlock());
>> -
>> - // See if we can do any tail merging on those.
>> - if (MergePotentials.size() >= 2)
>> - MadeChange |= TryTailMergeBlocks(nullptr, nullptr);
>> + // See if we can do any tail merging on those.
>> + if (MergePotentials.size() >= 2)
>> + MadeChange |= TryTailMergeBlocks(nullptr, nullptr); }
>>
>> // Look at blocks (IBB) with multiple predecessors (PBB).
>> // We change each predecessor to a canonical form, by @@ -997,6
>> +1016,17 @@ bool BranchFolder::TailMergeBlocks(Machi
>> if (PBB->hasEHPadSuccessor())
>> continue;
>>
>> + // Bail out if the loop header (IBB) is not the top of the loop
>> chain
>> + // after the block placement. Otherwise, the common tail of
>> IBB's
>> + // predecessors may become the loop top if block placement is
>> called again
>> + // and the predecessors may branch to this common tail.
>> + // FIXME: Relaxed this check if the algorithm of finding loop
>> top
>> is
>> + // changed in MBP.
>> + if (AfterBlockPlacement && MLI)
>> + if (MachineLoop *ML = MLI->getLoopFor(IBB))
>> + if (IBB == ML->getHeader() && ML == MLI->getLoopFor(PBB))
>> + continue;
>> +
>> MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
>> SmallVector<MachineOperand, 4> Cond;
>> if (!TII->AnalyzeBranch(*PBB, TBB, FBB, Cond, true)) {
>>
>> Modified: llvm/trunk/lib/CodeGen/BranchFolding.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/BranchFold
>> i ng.h?rev=271925&r1=271924&r2=271925&view=diff
>> =====================================================================
>> =
>> ========
>> --- llvm/trunk/lib/CodeGen/BranchFolding.h (original)
>> +++ llvm/trunk/lib/CodeGen/BranchFolding.h Mon Jun 6 13:36:07 2016
>> @@ -20,6 +20,7 @@ namespace llvm {
>> class MachineBranchProbabilityInfo;
>> class MachineFunction;
>> class MachineModuleInfo;
>> + class MachineLoopInfo;
>> class RegScavenger;
>> class TargetInstrInfo;
>> class TargetRegisterInfo;
>> @@ -32,10 +33,11 @@ namespace llvm {
>> MBFIWrapper &MBFI,
>> const MachineBranchProbabilityInfo &MBPI);
>>
>> - bool OptimizeFunction(MachineFunction &MF,
>> - const TargetInstrInfo *tii,
>> - const TargetRegisterInfo *tri,
>> - MachineModuleInfo *mmi);
>> + bool OptimizeFunction(MachineFunction &MF, const TargetInstrInfo
>> *tii,
>> + const TargetRegisterInfo *tri,
>> MachineModuleInfo *mmi,
>> + MachineLoopInfo *mli = nullptr,
>> + bool AfterPlacement = false);
>> +
>> private:
>> class MergePotentialsElt {
>> unsigned Hash;
>> @@ -93,11 +95,13 @@ namespace llvm {
>> };
>> std::vector<SameTailElt> SameTails;
>>
>> + bool AfterBlockPlacement;
>> bool EnableTailMerge;
>> bool EnableHoistCommonCode;
>> const TargetInstrInfo *TII;
>> const TargetRegisterInfo *TRI;
>> MachineModuleInfo *MMI;
>> + MachineLoopInfo *MLI;
>> RegScavenger *RS;
>>
>> public:
>>
>> Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlo
>> c kPlacement.cpp?rev=271925&r1=271924&r2=271925&view=diff
>> =====================================================================
>> =
>> ========
>> --- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Mon Jun 6
>> +++ 13:36:07 2016
>> @@ -26,6 +26,8 @@
>>
>> //===----------------------------------------------------------------
>> -
>> -----===//
>>
>> #include "llvm/CodeGen/Passes.h"
>> +#include "llvm/CodeGen/TargetPassConfig.h"
>> +#include "BranchFolding.h"
>> #include "llvm/ADT/DenseMap.h"
>> #include "llvm/ADT/SmallPtrSet.h"
>> #include "llvm/ADT/SmallVector.h"
>> @@ -116,6 +118,12 @@ static cl::opt<unsigned> JumpInstCost("j
>> cl::desc("Cost of jump
>> instructions."),
>> cl::init(1), cl::Hidden);
>>
>> +static cl::opt<bool>
>> +BranchFoldPlacement("branch-fold-placement",
>> + cl::desc("Perform branch folding during placement. "
>> + "Reduces code size."),
>> + cl::init(true), cl::Hidden);
>> +
>> extern cl::opt<unsigned> StaticLikelyProb;
>>
>> namespace {
>> @@ -232,10 +240,10 @@ class MachineBlockPlacement : public Mac
>> const MachineBranchProbabilityInfo *MBPI;
>>
>> /// \brief A handle to the function-wide block frequency pass.
>> - const MachineBlockFrequencyInfo *MBFI;
>> + std::unique_ptr<BranchFolder::MBFIWrapper> MBFI;
>>
>> /// \brief A handle to the loop info.
>> - const MachineLoopInfo *MLI;
>> + MachineLoopInfo *MLI;
>>
>> /// \brief A handle to the target's instruction info.
>> const TargetInstrInfo *TII;
>> @@ -323,6 +331,7 @@ public:
>> AU.addRequired<MachineBlockFrequencyInfo>();
>> AU.addRequired<MachineDominatorTree>();
>> AU.addRequired<MachineLoopInfo>();
>> + AU.addRequired<TargetPassConfig>();
>> MachineFunctionPass::getAnalysisUsage(AU);
>> }
>> };
>> @@ -1462,7 +1471,8 @@ bool MachineBlockPlacement::runOnMachine
>> return false;
>>
>> MBPI = &getAnalysis<MachineBranchProbabilityInfo>();
>> - MBFI = &getAnalysis<MachineBlockFrequencyInfo>();
>> + MBFI = llvm::make_unique<BranchFolder::MBFIWrapper>(
>> + getAnalysis<MachineBlockFrequencyInfo>());
>> MLI = &getAnalysis<MachineLoopInfo>();
>> TII = F.getSubtarget().getInstrInfo();
>> TLI = F.getSubtarget().getTargetLowering();
>> @@ -1470,6 +1480,29 @@ bool MachineBlockPlacement::runOnMachine
>> assert(BlockToChain.empty());
>>
>> buildCFGChains(F);
>> +
>> + // Changing the layout can create new tail merging opportunities.
>> + TargetPassConfig *PassConfig = &getAnalysis<TargetPassConfig>();
>> + // TailMerge can create jump into if branches that make CFG
>> + irreducible for // HW that requires structurized CFG.
>> + bool EnableTailMerge = !F.getTarget().requiresStructuredCFG() &&
>> + PassConfig->getEnableTailMerge() &&
>> + BranchFoldPlacement; // No tail merging
>> + opportunities if the block number is less than four.
>> + if (F.size() > 3 && EnableTailMerge) {
>> + BranchFolder BF(/*EnableTailMerge=*/true, /*CommonHoist=*/false,
>> *MBFI,
>> + *MBPI);
>> +
>> + if (BF.OptimizeFunction(F, TII,
>> F.getSubtarget().getRegisterInfo(),
>> +
>> getAnalysisIfAvailable<MachineModuleInfo>(),
>> MLI,
>> + /*AfterBlockPlacement=*/true)) {
>> + // Redo the layout if tail merging creates/removes/moves
>> blocks.
>> + BlockToChain.clear();
>> + ChainAllocator.DestroyAll();
>> + buildCFGChains(F);
>> + }
>> + }
>> +
>> optimizeBranches(F);
>> alignBlocks(F);
>>
>>
>> Added: llvm/trunk/test/CodeGen/AArch64/tailmerging_in_mbp.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/t
>> a ilmerging_in_mbp.ll?rev=271925&view=auto
>> =====================================================================
>> =
>> ========
>> --- llvm/trunk/test/CodeGen/AArch64/tailmerging_in_mbp.ll (added)
>> +++ llvm/trunk/test/CodeGen/AArch64/tailmerging_in_mbp.ll Mon Jun 6
>> +++ 13:36:07 2016
>> @@ -0,0 +1,63 @@
>> +; RUN: llc <%s -march=aarch64 | FileCheck %s
>> +
>> +; CHECK-LABEL: test:
>> +; CHECK: .LBB0_7
>> +; CHECK: b.hi .LBB0_2
>> +; CHECK-NEXT: b .LBB0_9
>> +; CHECK-NEXT: .LBB0_8
>> +; CHECK-NEXT: mov x8, x9
>> +; CHECK-NEXT: .LBB0_9
>> +define i64 @test(i64 %n, i64* %a, i64* %b, i64* %c, i64* %d, i64*
>> +%e,
>> +i64* %f) {
>> +entry:
>> + %cmp28 = icmp sgt i64 %n, 1
>> + br i1 %cmp28, label %for.body, label %for.end
>> +
>> +for.body: ; preds =
>> %for.body.lr.ph, %if.end
>> + %j = phi i64 [ %n, %entry ], [ %div, %if.end ]
>> + %div = lshr i64 %j, 1
>> + %a.arrayidx = getelementptr inbounds i64, i64* %a, i64 %div
>> + %a.j = load i64, i64* %a.arrayidx
>> + %b.arrayidx = getelementptr inbounds i64, i64* %b, i64 %div
>> + %b.j = load i64, i64* %b.arrayidx
>> + %cmp.i = icmp slt i64 %a.j, %b.j
>> + br i1 %cmp.i, label %for.end.loopexit, label %cond.false.i
>> +
>> +cond.false.i: ; preds = %for.body
>> + %cmp4.i = icmp sgt i64 %a.j, %b.j
>> + br i1 %cmp4.i, label %if.end, label %cond.false6.i
>> +
>> +cond.false6.i: ; preds =
>> %cond.false.i
>> + %c.arrayidx = getelementptr inbounds i64, i64* %c, i64 %div
>> + %c.j = load i64, i64* %c.arrayidx
>> + %d.arrayidx = getelementptr inbounds i64, i64* %d, i64 %div
>> + %d.j = load i64, i64* %d.arrayidx
>> + %cmp9.i = icmp slt i64 %c.j, %d.j
>> + br i1 %cmp9.i, label %for.end.loopexit, label %cond.false11.i
>> +
>> +cond.false11.i: ; preds =
>> %cond.false6.i
>> + %cmp14.i = icmp sgt i64 %c.j, %d.j
>> + br i1 %cmp14.i, label %if.end, label %cond.false12.i
>> +
>> +cond.false12.i: ; preds = %cond.false11.i
>> + %e.arrayidx = getelementptr inbounds i64, i64* %e, i64 %div
>> + %e.j = load i64, i64* %e.arrayidx
>> + %f.arrayidx = getelementptr inbounds i64, i64* %f, i64 %div
>> + %f.j = load i64, i64* %f.arrayidx
>> + %cmp19.i = icmp sgt i64 %e.j, %f.j
>> + br i1 %cmp19.i, label %if.end, label %for.end.loopexit
>> +
>> +if.end: ; preds =
>> %cond.false12.i, %cond.false11.i, %cond.false.i
>> + %cmp = icmp ugt i64 %j, 3
>> + br i1 %cmp, label %for.body, label %for.end.loopexit
>> +
>> +for.end.loopexit: ; preds =
>> %cond.false12.i, %cond.false6.i, %for.body, %if.end
>> + %j.0.lcssa.ph = phi i64 [ %j, %cond.false12.i ], [ %j,
>> +%cond.false6.i ], [ %j, %for.body ], [ %div, %if.end ]
>> + br label %for.end
>> +
>> +for.end: ; preds =
>> %for.end.loopexit, %entry
>> + %j.0.lcssa = phi i64 [ %n, %entry ], [ %j.0.lcssa.ph,
>> +%for.end.loopexit ]
>> + %j.2 = add i64 %j.0.lcssa, %n
>> + %j.3 = mul i64 %j.2, %n
>> + %j.4 = add i64 %j.3, 10
>> + ret i64 %j.4
>> +}
>>
>> Modified: llvm/trunk/test/CodeGen/ARM/arm-and-tst-peephole.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/arm-a
>> n d-tst-peephole.ll?rev=271925&r1=271924&r2=271925&view=diff
>> =====================================================================
>> =
>> ========
>> --- llvm/trunk/test/CodeGen/ARM/arm-and-tst-peephole.ll (original)
>> +++ llvm/trunk/test/CodeGen/ARM/arm-and-tst-peephole.ll Mon Jun 6
>> +++ 13:36:07 2016
>> @@ -49,7 +49,7 @@ tailrecurse.switch:
>> ; V8-NEXT: beq
>> ; V8-NEXT: %tailrecurse.switch
>> ; V8: cmp
>> -; V8-NEXT: bne
>> +; V8-NEXT: beq
>> ; V8-NEXT: b
>> ; The trailing space in the last line checks that the branch is
>> unconditional
>> switch i32 %and, label %sw.epilog [
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list