[PATCH] Fix information loss in branch probability computation.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Thu May 7 08:44:22 PDT 2015
> On 2015 May 7, at 11:26, Diego Novillo <dnovillo at google.com> wrote:
>
> This version fixes the 32bit overflow check in visitSwitch()
>
>
> http://reviews.llvm.org/D9442
>
> Files:
> lib/Analysis/BranchProbabilityInfo.cpp
> lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> test/Analysis/BranchProbabilityInfo/pr22718.ll
> test/CodeGen/X86/MachineBranchProb.ll
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
> <D9442.25192.patch>
I think I covered this in my other email, but just in case:
> Index: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> +++ lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> @@ -8004,6 +8004,7 @@
> BranchProbabilityInfo *BPI = FuncInfo.BPI;
> CaseClusterVector Clusters;
> Clusters.reserve(SI.getNumCases());
> + uint64_t WeightSum = 0;
> for (auto I : SI.cases()) {
> MachineBasicBlock *Succ = FuncInfo.MBBMap[I.getCaseSuccessor()];
> const ConstantInt *CaseVal = I.getCaseValue();
> @@ -8014,10 +8015,11 @@
> // here, use the same flooring mechanism previously used by BPI.
> Weight = std::max(
> 1u, BPI->getEdgeWeight(SI.getParent(), I.getSuccessorIndex()));
> - assert(Weight <= UINT32_MAX / SI.getNumSuccessors());
> + WeightSum += Weight;
> }
> Clusters.push_back(CaseCluster::range(CaseVal, CaseVal, Succ, Weight));
> }
> + assert(WeightSum <= UINT32_MAX && "Branch weights exceed 32bit limit");
This assertion will fire under circumstances such as:
- `1` successor has a weight of `3`.
- `UINT32_MAX - 2` successors have a weight of `0` each.
This won't get rescaled by the new BPI, but it violates the local
assumptions (with the floor, the sum will be `UINT32_MAX + 1`).
>
> MachineBasicBlock *DefaultMBB = FuncInfo.MBBMap[SI.getDefaultDest()];
>
More information about the llvm-commits
mailing list