[llvm] r244154 - Record whether the weights on out-edges from a MBB are normalized.
Cong Hou via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 6 12:24:51 PDT 2015
Hi Vasileios
I have tried your test cases and they both failed even -fast-isel=true
is not indicated. I did an investigation and found the issue is quite
subtle: for zero edge weights, MBPI will instead use a default edge
weight (currently it is 16). This will prevent the edge weight and
hence the sum of edge weights to be zero. My patch breaks this rule
and leads to the assertion failure (denominator cannot be zero).
However with the rule when you pass 0 and 1 as edge weights, they will
become 16 and 1, which may not be the intention of the user.
I have reverted my commit, and will discuss this issue with the author
of MBPI. Thank you very much for reporting the issue!
thanks,
Cong
On Thu, Aug 6, 2015 at 7:59 AM, Vasileios Kalintiris
<Vasileios.Kalintiris at imgtec.com> wrote:
> Hi Hou,
>
> This commit broke 3 tests in our out-of-tree buildbots for MIPS. The failures occur when FastISel is explicitly enabled under -O2 (llc -O2 -fast-isel=true < foo.ll) . I've attached the simplified IR from bugpoint for two tests. Could you take a look?
>
> Thanks,
> V. Kalintiris
>
> ________________________________________
> From: llvm-commits [llvm-commits-bounces at lists.llvm.org] on behalf of Cong Hou [congh at google.com]
> Sent: 05 August 2015 23:01
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] r244154 - Record whether the weights on out-edges from a MBB are normalized.
>
> Author: conghou
> Date: Wed Aug 5 17:01:20 2015
> New Revision: 244154
>
> URL: http://llvm.org/viewvc/llvm-project?rev=244154&view=rev
> Log:
> Record whether the weights on out-edges from a MBB are normalized.
>
> 1. Create a utility function normalizeEdgeWeights() in MachineBranchProbabilityInfo that normalizes a list of edge weights so that the sum of then can fit in uint32_t.
> 2. Provide an interface in MachineBasicBlock to normalize its successors' weights.
> 3. Add a flag in MachineBasicBlock that tracks whether its successors' weights are normalized.
> 4. Provide an overload of getSumForBlock that accepts a non-const pointer to a MBB so that it can force normalizing this MBB's successors' weights.
> 5. Update several uses of getSumForBlock() by eliminating the once needed weight scale.
>
> Differential Revision: http://reviews.llvm.org/D11442
>
>
> Modified:
> llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h
> llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> llvm/trunk/lib/CodeGen/IfConversion.cpp
> llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
> llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
> llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h?rev=244154&r1=244153&r2=244154&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h Wed Aug 5 17:01:20 2015
> @@ -65,6 +65,10 @@ class MachineBasicBlock : public ilist_n
> Instructions Insts;
> const BasicBlock *BB;
> int Number;
> +
> + /// A flag tracking whether the weights of all successors are normalized.
> + bool AreSuccWeightsNormalized;
> +
> MachineFunction *xParent;
>
> /// Keep track of the predecessor / successor basicblocks.
> @@ -129,6 +133,9 @@ public:
> const MachineFunction *getParent() const { return xParent; }
> MachineFunction *getParent() { return xParent; }
>
> + /// Return whether all weights of successors are normalized.
> + bool areSuccWeightsNormalized() const { return AreSuccWeightsNormalized; }
> +
> /// MachineBasicBlock iterator that automatically skips over MIs that are
> /// inside bundles (i.e. walk top level MIs only).
> template<typename Ty, typename IterTy>
> @@ -384,6 +391,12 @@ public:
> /// Set successor weight of a given iterator.
> void setSuccWeight(succ_iterator I, uint32_t weight);
>
> + /// Normalize all succesor weights so that the sum of them does not exceed
> + /// UINT32_MAX. Return true if the weights are modified and false otherwise.
> + /// Note that weights that are modified after calling this function are not
> + /// guaranteed to be normalized.
> + bool normalizeSuccWeights();
> +
> /// Remove successor from the successors list of this MachineBasicBlock. The
> /// Predecessors list of succ is automatically updated.
> void removeSuccessor(MachineBasicBlock *succ);
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h?rev=244154&r1=244153&r2=244154&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h Wed Aug 5 17:01:20 2015
> @@ -59,6 +59,10 @@ public:
> // adjustment. Any edge weights used with the sum should be divided by Scale.
> uint32_t getSumForBlock(const MachineBasicBlock *MBB, uint32_t &Scale) const;
>
> + // Get sum of the block successors' weights, and force normalizing the
> + // successors' weights of MBB so that their sum fit within 32-bits.
> + uint32_t getSumForBlock(MachineBasicBlock *MBB) const;
> +
> // A 'Hot' edge is an edge which probability is >= 80%.
> bool isEdgeHot(const MachineBasicBlock *Src,
> const MachineBasicBlock *Dst) const;
> @@ -82,8 +86,34 @@ public:
> raw_ostream &printEdgeProbability(raw_ostream &OS,
> const MachineBasicBlock *Src,
> const MachineBasicBlock *Dst) const;
> +
> + // Normalize a list of weights by scaling them down so that the sum of them
> + // doesn't exceed UINT32_MAX. Return the scale.
> + template <class WeightList>
> + static uint32_t normalizeEdgeWeights(WeightList &Weights);
> };
>
> +template <class WeightList>
> +uint32_t
> +MachineBranchProbabilityInfo::normalizeEdgeWeights(WeightList &Weights) {
> + assert(Weights.size() < UINT32_MAX && "Too many weights in the list!");
> + // First we compute the sum with 64-bits of precision.
> + uint64_t Sum = std::accumulate(Weights.begin(), Weights.end(), uint64_t(0));
> +
> + // If the computed sum fits in 32-bits, we're done.
> + if (Sum <= UINT32_MAX)
> + return 1;
> +
> + // Otherwise, compute the scale necessary to cause the weights to fit, and
> + // re-sum with that scale applied.
> + assert((Sum / UINT32_MAX) < UINT32_MAX &&
> + "The sum of weights exceeds UINT32_MAX^2!");
> + uint32_t Scale = (Sum / UINT32_MAX) + 1;
> + for (auto &W : Weights)
> + W /= Scale;
> + return Scale;
> +}
> +
> }
>
>
>
> Modified: llvm/trunk/lib/CodeGen/IfConversion.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/IfConversion.cpp?rev=244154&r1=244153&r2=244154&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/IfConversion.cpp (original)
> +++ llvm/trunk/lib/CodeGen/IfConversion.cpp Wed Aug 5 17:01:20 2015
> @@ -1232,15 +1232,17 @@ bool IfConverter::IfConvertTriangle(BBIn
>
> bool HasEarlyExit = CvtBBI->FalseBB != nullptr;
> uint64_t CvtNext = 0, CvtFalse = 0, BBNext = 0, BBCvt = 0, SumWeight = 0;
> - uint32_t WeightScale = 0;
>
> if (HasEarlyExit) {
> // Get weights before modifying CvtBBI->BB and BBI.BB.
> + // Explictly normalize the weights of all edges from CvtBBI->BB so that we
> + // are aware that the edge weights obtained below are normalized.
> + CvtBBI->BB->normalizeSuccWeights();
> CvtNext = MBPI->getEdgeWeight(CvtBBI->BB, NextBBI->BB);
> CvtFalse = MBPI->getEdgeWeight(CvtBBI->BB, CvtBBI->FalseBB);
> BBNext = MBPI->getEdgeWeight(BBI.BB, NextBBI->BB);
> BBCvt = MBPI->getEdgeWeight(BBI.BB, CvtBBI->BB);
> - SumWeight = MBPI->getSumForBlock(CvtBBI->BB, WeightScale);
> + SumWeight = MBPI->getSumForBlock(CvtBBI->BB);
> }
>
> if (CvtBBI->BB->pred_size() > 1) {
> @@ -1277,8 +1279,8 @@ bool IfConverter::IfConvertTriangle(BBIn
> // New_Weight(BBI.BB, CvtBBI->FalseBB) =
> // Weight(BBI.BB, CvtBBI->BB) * Weight(CvtBBI->BB, CvtBBI->FalseBB)
>
> - uint64_t NewNext = BBNext * SumWeight + (BBCvt * CvtNext) / WeightScale;
> - uint64_t NewFalse = (BBCvt * CvtFalse) / WeightScale;
> + uint64_t NewNext = BBNext * SumWeight + BBCvt * CvtNext;
> + uint64_t NewFalse = BBCvt * CvtFalse;
> // We need to scale down all weights of BBI.BB to fit uint32_t.
> // Here BBI.BB is connected to CvtBBI->FalseBB and will fall through to
> // the next block.
>
> Modified: llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp?rev=244154&r1=244153&r2=244154&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp Wed Aug 5 17:01:20 2015
> @@ -16,6 +16,7 @@
> #include "llvm/ADT/SmallString.h"
> #include "llvm/CodeGen/LiveIntervalAnalysis.h"
> #include "llvm/CodeGen/LiveVariables.h"
> +#include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
> #include "llvm/CodeGen/MachineDominators.h"
> #include "llvm/CodeGen/MachineFunction.h"
> #include "llvm/CodeGen/MachineInstrBuilder.h"
> @@ -39,8 +40,9 @@ using namespace llvm;
> #define DEBUG_TYPE "codegen"
>
> MachineBasicBlock::MachineBasicBlock(MachineFunction &mf, const BasicBlock *bb)
> - : BB(bb), Number(-1), xParent(&mf), Alignment(0), IsLandingPad(false),
> - AddressTaken(false), CachedMCSymbol(nullptr) {
> + : BB(bb), Number(-1), AreSuccWeightsNormalized(false), xParent(&mf),
> + Alignment(0), IsLandingPad(false), AddressTaken(false),
> + CachedMCSymbol(nullptr) {
> Insts.Parent = this;
> }
>
> @@ -481,8 +483,10 @@ void MachineBasicBlock::addSuccessor(Mac
> if (weight != 0 && Weights.empty())
> Weights.resize(Successors.size());
>
> - if (weight != 0 || !Weights.empty())
> + if (weight != 0 || !Weights.empty()) {
> Weights.push_back(weight);
> + AreSuccWeightsNormalized = false;
> + }
>
> Successors.push_back(succ);
> succ->addPredecessor(this);
> @@ -1096,7 +1100,25 @@ uint32_t MachineBasicBlock::getSuccWeigh
> void MachineBasicBlock::setSuccWeight(succ_iterator I, uint32_t weight) {
> if (Weights.empty())
> return;
> - *getWeightIterator(I) = weight;
> + auto WeightIter = getWeightIterator(I);
> + uint32_t OldWeight = *WeightIter;
> + *WeightIter = weight;
> + if (weight > OldWeight)
> + AreSuccWeightsNormalized = false;
> +}
> +
> +/// Normalize all succesor weights so that the sum of them does not exceed
> +/// UINT32_MAX. Return true if the weights are modified and false otherwise.
> +/// Note that weights that are modified after calling this function are not
> +/// guaranteed to be normalized.
> +bool MachineBasicBlock::normalizeSuccWeights() {
> + if (!AreSuccWeightsNormalized) {
> + uint32_t Scale =
> + MachineBranchProbabilityInfo::normalizeEdgeWeights(Weights);
> + AreSuccWeightsNormalized = true;
> + return Scale != 1;
> + }
> + return false;
> }
>
> /// getWeightIterator - Return wight iterator corresonding to the I successor
>
> Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp?rev=244154&r1=244153&r2=244154&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Wed Aug 5 17:01:20 2015
> @@ -361,8 +361,7 @@ MachineBlockPlacement::selectBestSuccess
> // improve the MBPI interface to efficiently support query patterns such as
> // this.
> uint32_t BestWeight = 0;
> - uint32_t WeightScale = 0;
> - uint32_t SumWeight = MBPI->getSumForBlock(BB, WeightScale);
> + uint32_t SumWeight = MBPI->getSumForBlock(BB);
> DEBUG(dbgs() << "Attempting merge from: " << getBlockName(BB) << "\n");
> for (MachineBasicBlock *Succ : BB->successors()) {
> if (BlockFilter && !BlockFilter->count(Succ))
> @@ -378,7 +377,7 @@ MachineBlockPlacement::selectBestSuccess
> }
>
> uint32_t SuccWeight = MBPI->getEdgeWeight(BB, Succ);
> - BranchProbability SuccProb(SuccWeight / WeightScale, SumWeight);
> + BranchProbability SuccProb(SuccWeight, SumWeight);
>
> // If we outline optional branches, look whether Succ is unavoidable, i.e.
> // dominates all terminators of the MachineFunction. If it does, other
> @@ -675,8 +674,7 @@ MachineBlockPlacement::findBestLoopExit(
> // FIXME: Due to the performance of the probability and weight routines in
> // the MBPI analysis, we use the internal weights and manually compute the
> // probabilities to avoid quadratic behavior.
> - uint32_t WeightScale = 0;
> - uint32_t SumWeight = MBPI->getSumForBlock(MBB, WeightScale);
> + uint32_t SumWeight = MBPI->getSumForBlock(MBB);
> for (MachineBasicBlock *Succ : MBB->successors()) {
> if (Succ->isLandingPad())
> continue;
> @@ -705,7 +703,7 @@ MachineBlockPlacement::findBestLoopExit(
> BlocksExitingToOuterLoop.insert(MBB);
> }
>
> - BranchProbability SuccProb(SuccWeight / WeightScale, SumWeight);
> + BranchProbability SuccProb(SuccWeight, SumWeight);
> BlockFrequency ExitEdgeFreq = MBFI->getBlockFreq(MBB) * SuccProb;
> DEBUG(dbgs() << " exiting: " << getBlockName(MBB) << " -> "
> << getBlockName(Succ) << " [L:" << SuccLoopDepth << "] (";
>
> Modified: llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp?rev=244154&r1=244153&r2=244154&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp Wed Aug 5 17:01:20 2015
> @@ -28,36 +28,35 @@ char MachineBranchProbabilityInfo::ID =
>
> void MachineBranchProbabilityInfo::anchor() { }
>
> -uint32_t MachineBranchProbabilityInfo::
> -getSumForBlock(const MachineBasicBlock *MBB, uint32_t &Scale) const {
> - // First we compute the sum with 64-bits of precision, ensuring that cannot
> - // overflow by bounding the number of weights considered. Hopefully no one
> - // actually needs 2^32 successors.
> - assert(MBB->succ_size() < UINT32_MAX);
> - uint64_t Sum = 0;
> - Scale = 1;
> +uint32_t
> +MachineBranchProbabilityInfo::getSumForBlock(MachineBasicBlock *MBB) const {
> + // Normalize the weights of MBB's all successors so that the sum is guaranteed
> + // to be no greater than UINT32_MAX.
> + MBB->normalizeSuccWeights();
> +
> + SmallVector<uint32_t, 8> Weights;
> for (MachineBasicBlock::const_succ_iterator I = MBB->succ_begin(),
> - E = MBB->succ_end(); I != E; ++I) {
> - uint32_t Weight = getEdgeWeight(MBB, I);
> - Sum += Weight;
> - }
> -
> - // If the computed sum fits in 32-bits, we're done.
> - if (Sum <= UINT32_MAX)
> - return Sum;
> -
> - // Otherwise, compute the scale necessary to cause the weights to fit, and
> - // re-sum with that scale applied.
> - assert((Sum / UINT32_MAX) < UINT32_MAX);
> - Scale = (Sum / UINT32_MAX) + 1;
> - Sum = 0;
> + E = MBB->succ_end();
> + I != E; ++I)
> + Weights.push_back(getEdgeWeight(MBB, I));
> +
> + return std::accumulate(Weights.begin(), Weights.end(), 0u);
> +}
> +
> +uint32_t
> +MachineBranchProbabilityInfo::getSumForBlock(const MachineBasicBlock *MBB,
> + uint32_t &Scale) const {
> + SmallVector<uint32_t, 8> Weights;
> for (MachineBasicBlock::const_succ_iterator I = MBB->succ_begin(),
> - E = MBB->succ_end(); I != E; ++I) {
> - uint32_t Weight = getEdgeWeight(MBB, I);
> - Sum += Weight / Scale;
> - }
> - assert(Sum <= UINT32_MAX);
> - return Sum;
> + E = MBB->succ_end();
> + I != E; ++I)
> + Weights.push_back(getEdgeWeight(MBB, I));
> +
> + if (MBB->areSuccWeightsNormalized())
> + Scale = 1;
> + else
> + Scale = MachineBranchProbabilityInfo::normalizeEdgeWeights(Weights);
> + return std::accumulate(Weights.begin(), Weights.end(), 0u);
> }
>
> uint32_t MachineBranchProbabilityInfo::
>
>
> _______________________________________________
> 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