[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