[llvm-commits] [llvm] r144526 - in /llvm/trunk: include/llvm/CodeGen/MachineBranchProbabilityInfo.h lib/CodeGen/MachineBranchProbabilityInfo.cpp test/CodeGen/X86/block-placement.ll
Andrew Trick
atrick at apple.com
Tue Nov 15 11:07:36 PST 2011
On Nov 14, 2011, at 12:50 AM, Chandler Carruth wrote:
> Author: chandlerc
> Date: Mon Nov 14 02:50:16 2011
> New Revision: 144526
>
> URL: http://llvm.org/viewvc/llvm-project?rev=144526&view=rev
> Log:
> Fix an overflow bug in MachineBranchProbabilityInfo. This pass relied on
> the sum of the edge weights not overflowing uint32, and crashed when
> they did. This is generally safe as BranchProbabilityInfo tries to
> provide this guarantee. However, the CFG can get modified during codegen
> in a way that grows the *sum* of the edge weights. This doesn't seem
> unreasonable (imagine just adding more blocks all with the default
> weight of 16), but it is hard to come up with a case that actually
> triggers 32-bit overflow. Fortuately, the single-source GCC build is
> good at this. The solution isn't very pretty, but its no worse than the
> previous code. We're already summing all of the edge weights on each
> query, we can sum them, check for an overflow, compute a scale, and sum
> them again.
>
> I've included a *greatly* reduced test case out of the GCC source that
> triggers it. It's a pretty lame test, as it clearly is just barely
> triggering the overflow. I'd like to have something that is much more
> definitive, but I don't understand the fundamental pattern that triggers
> an explosion in the edge weight sums.
>
> The buggy code is duplicated within this file. I'll colapse them into
> a single implementation in a subsequent commit.
When we discussed this design, we wanted that scaling to happen when edges are added. Hence the original assert checking for overflow. But, as you know, none of the CFG update logic was implemented. Your approach seems fine though, since it's a simple check and rarely hit. But did you consider handling this within addSuccessor? If we did that, we would ned weight_iterator to be const_iterator as well.
-Andy
>
> Modified:
> llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp
> llvm/trunk/test/CodeGen/X86/block-placement.ll
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h?rev=144526&r1=144525&r2=144526&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineBranchProbabilityInfo.h Mon Nov 14 02:50:16 2011
> @@ -34,8 +34,10 @@
> // weight to just "inherit" the non-zero weight of an adjacent successor.
> static const uint32_t DEFAULT_WEIGHT = 16;
>
> - // Get sum of the block successors' weights.
> - uint32_t getSumForBlock(MachineBasicBlock *MBB) const;
> + // Get sum of the block successors' weights, potentially scaling them to fit
> + // within 32-bits. If scaling is required, sets Scale based on the necessary
> + // adjustment. Any edge weights used with the sum should be divided by Scale.
> + uint32_t getSumForBlock(MachineBasicBlock *MBB, uint32_t &Scale) const;
>
> public:
> static char ID;
>
> Modified: llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp?rev=144526&r1=144525&r2=144526&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineBranchProbabilityInfo.cpp Mon Nov 14 02:50:16 2011
> @@ -27,19 +27,34 @@
> char MachineBranchProbabilityInfo::ID = 0;
>
> uint32_t MachineBranchProbabilityInfo::
> -getSumForBlock(MachineBasicBlock *MBB) const {
> - uint32_t Sum = 0;
> -
> +getSumForBlock(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;
> for (MachineBasicBlock::const_succ_iterator I = MBB->succ_begin(),
> E = MBB->succ_end(); I != E; ++I) {
> - MachineBasicBlock *Succ = *I;
> - uint32_t Weight = getEdgeWeight(MBB, Succ);
> - uint32_t PrevSum = Sum;
> -
> + uint32_t Weight = getEdgeWeight(MBB, *I);
> Sum += Weight;
> - assert(Sum > PrevSum); (void) PrevSum;
> }
>
> + // 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;
> + 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;
> }
>
> @@ -89,8 +104,9 @@
> BranchProbability
> MachineBranchProbabilityInfo::getEdgeProbability(MachineBasicBlock *Src,
> MachineBasicBlock *Dst) const {
> - uint32_t N = getEdgeWeight(Src, Dst);
> - uint32_t D = getSumForBlock(Src);
> + uint32_t Scale = 1;
> + uint32_t D = getSumForBlock(Src, Scale);
> + uint32_t N = getEdgeWeight(Src, Dst) / Scale;
>
> return BranchProbability(N, D);
> }
>
> Modified: llvm/trunk/test/CodeGen/X86/block-placement.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/block-placement.ll?rev=144526&r1=144525&r2=144526&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/block-placement.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/block-placement.ll Mon Nov 14 02:50:16 2011
> @@ -271,3 +271,54 @@
> %ptr2 = load i32** undef, align 4
> br label %loop.body3
> }
> +
> +define i32 @problematic_switch() {
> +; This function's CFG caused overlow in the machine branch probability
> +; calculation, triggering asserts. Make sure we don't crash on it.
> +; CHECK: problematic_switch
> +
> +entry:
> + switch i32 undef, label %exit [
> + i32 879, label %bogus
> + i32 877, label %step
> + i32 876, label %step
> + i32 875, label %step
> + i32 874, label %step
> + i32 873, label %step
> + i32 872, label %step
> + i32 868, label %step
> + i32 867, label %step
> + i32 866, label %step
> + i32 861, label %step
> + i32 860, label %step
> + i32 856, label %step
> + i32 855, label %step
> + i32 854, label %step
> + i32 831, label %step
> + i32 830, label %step
> + i32 829, label %step
> + i32 828, label %step
> + i32 815, label %step
> + i32 814, label %step
> + i32 811, label %step
> + i32 806, label %step
> + i32 805, label %step
> + i32 804, label %step
> + i32 803, label %step
> + i32 802, label %step
> + i32 801, label %step
> + i32 800, label %step
> + i32 799, label %step
> + i32 798, label %step
> + i32 797, label %step
> + i32 796, label %step
> + i32 795, label %step
> + ]
> +bogus:
> + unreachable
> +step:
> + br label %exit
> +exit:
> + %merge = phi i32 [ 3, %step ], [ 6, %entry ]
> + ret i32 %merge
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list