[llvm] r226600 - Prevent binary-tree deterioration in sparse switch statements.

[Bruno Cardoso Lopes]

Hi Daniel,

I've tried the exact same approach back on Sep 2014 but I got a ~4%
performance degradation in
External/SPEC/CINT2006/462_libquantum/462_libquantum. Back in the day
it would affect switch codegen in the  "quantum_objcode_run" function,
in objcode.c => jump tables and a bunch of code started to be emitted
after the patch.

Since it has been a while, much could have changed and that could no
longer apply.
Have you measured the impact of this on the test-suite?

Thanks,

On Tue, Jan 20, 2015 at 5:43 PM, Daniel Jasper <djasper at google.com> wrote:
> Author: djasper
> Date: Tue Jan 20 13:43:33 2015
> New Revision: 226600
>
> URL: http://llvm.org/viewvc/llvm-project?rev=226600&view=rev
> Log:
> Prevent binary-tree deterioration in sparse switch statements.
>
> This addresses part of llvm.org/PR22262. Specifically, it prevents
> considering the densities of sub-ranges that have fewer than
> TLI.getMinimumJumpTableEntries() elements. Those densities won't help
> jump tables.
>
> This is not a complete solution but works around the most pressing
> issue.
>
> Review: http://reviews.llvm.org/D7070
>
> Modified:
>     llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>     llvm/trunk/test/CodeGen/X86/switch-bt.ll
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp?rev=226600&r1=226599&r2=226600&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Tue Jan 20 13:43:33 2015
> @@ -2420,6 +2420,7 @@ bool SelectionDAGBuilder::handleBTSplitS
>    DEBUG(dbgs() << "Selecting best pivot: \n"
>                 << "First: " << First << ", Last: " << Last <<'\n'
>                 << "LSize: " << LSize << ", RSize: " << RSize << '\n');
> +  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
>    for (CaseItr I = CR.Range.first, J=I+1, E = CR.Range.second;
>         J!=E; ++I, ++J) {
>      const APInt &LEnd = cast<ConstantInt>(I->High)->getValue();
> @@ -2429,10 +2430,16 @@ bool SelectionDAGBuilder::handleBTSplitS
>             "Invalid case distance");
>      // Use volatile double here to avoid excess precision issues on some hosts,
>      // e.g. that use 80-bit X87 registers.
> +    // Only consider the density of sub-ranges that actually have sufficient
> +    // entries to be lowered as a jump table.
>      volatile double LDensity =
> -        LSize.roundToDouble() / (LEnd - First + 1ULL).roundToDouble();
> +        LSize.ult(TLI.getMinimumJumpTableEntries())
> +            ? 0.0
> +            : LSize.roundToDouble() / (LEnd - First + 1ULL).roundToDouble();
>      volatile double RDensity =
> -        RSize.roundToDouble() / (Last - RBegin + 1ULL).roundToDouble();
> +        RSize.ult(TLI.getMinimumJumpTableEntries())
> +            ? 0.0
> +            : RSize.roundToDouble() / (Last - RBegin + 1ULL).roundToDouble();
>      volatile double Metric = Range.logBase2() * (LDensity + RDensity);
>      // Should always split in some non-trivial place
>      DEBUG(dbgs() <<"=>Step\n"
> @@ -2450,13 +2457,8 @@ bool SelectionDAGBuilder::handleBTSplitS
>      RSize -= J->size();
>    }
>
> -  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
> -  if (areJTsAllowed(TLI)) {
> -    // If our case is dense we *really* should handle it earlier!
> -    assert((FMetric > 0) && "Should handle dense range earlier!");
> -  } else {
> +  if (FMetric == 0 || !areJTsAllowed(TLI))
>      Pivot = CR.Range.first + Size/2;
> -  }
>    splitSwitchCase(CR, Pivot, WorkList, SV, SwitchBB);
>    return true;
>  }
>
> Modified: llvm/trunk/test/CodeGen/X86/switch-bt.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/switch-bt.ll?rev=226600&r1=226599&r2=226600&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/switch-bt.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/switch-bt.ll Tue Jan 20 13:43:33 2015
> @@ -99,3 +99,61 @@ if.then:
>  if.end:
>    ret void
>  }
> +
> +; Ensure that optimizing for jump tables doesn't needlessly deteriorate the
> +; created binary tree search. See PR22262.
> +define void @test4(i32 %x, i32* %y) {
> +; CHECK-LABEL: test4:
> +
> +entry:
> +  switch i32 %x, label %sw.default [
> +    i32 10, label %sw.bb
> +    i32 20, label %sw.bb1
> +    i32 30, label %sw.bb2
> +    i32 40, label %sw.bb3
> +    i32 50, label %sw.bb4
> +    i32 60, label %sw.bb5
> +  ]
> +sw.bb:
> +  store i32 1, i32* %y
> +  br label %sw.epilog
> +sw.bb1:
> +  store i32 2, i32* %y
> +  br label %sw.epilog
> +sw.bb2:
> +  store i32 3, i32* %y
> +  br label %sw.epilog
> +sw.bb3:
> +  store i32 4, i32* %y
> +  br label %sw.epilog
> +sw.bb4:
> +  store i32 5, i32* %y
> +  br label %sw.epilog
> +sw.bb5:
> +  store i32 6, i32* %y
> +  br label %sw.epilog
> +sw.default:
> +  store i32 7, i32* %y
> +  br label %sw.epilog
> +sw.epilog:
> +  ret void
> +
> +; The balanced binary switch here would start with a comparison against 39, but
> +; it is currently starting with 29 because of the density-sum heuristic.
> +; CHECK: cmpl $29
> +; CHECK: jg
> +; CHECK: cmpl $10
> +; CHECK: jne
> +; CHECK: cmpl $49
> +; CHECK: jg
> +; CHECK: cmpl $30
> +; CHECK: jne
> +; CHECK: cmpl $20
> +; CHECK: jne
> +; CHECK: cmpl $50
> +; CHECK: jne
> +; CHECK: cmpl $40
> +; CHECK: jne
> +; CHECK: cmpl $60
> +; CHECK: jne
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits



-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc