[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Pete Cooper]

Hey Marcello

Very minor nitpick, but can you fix the formatting of this code

+  DEBUG(dbgs() << "LHS Bounds ==> ";
+        if (LowerBound) {
+          dbgs() << cast<ConstantInt>(LowerBound)->getSExtValue();
+        } else { dbgs() << "NONE"; } dbgs()
+            << " - " << NewUpperBound->getSExtValue() << "\n";
+        dbgs() << "RHS Bounds ==> ";
+        dbgs() << NewLowerBound->getSExtValue() << " - "; if (UpperBound) {
+          dbgs() << cast<ConstantInt>(UpperBound)->getSExtValue() << "\n";
+        } else { dbgs() << "NONE\n"; });

Otherwise LGTM, pending David ok’ing your test updates.

Thanks,
Pete
On Jun 10, 2014, at 9:15 AM, Marcello Maggioni <hayarms at gmail.com> wrote:

> Here is a patch with the original test 'feature.ll' modified to pass on the new implementation
> 
> Marcello
> 
> 
> 2014-06-10 17:06 GMT+01:00 Marcello Maggioni <hayarms at gmail.com>:
> The new test is basically the old test updated to pass on the optimized algorithm.
> 
> I'll produce a patch with the new test renamed as the old test.
> 
> Marcello
> 
> 
> 2014-06-10 16:19 GMT+01:00 David Blaikie <dblaikie at gmail.com>:
> 
> On Tue, Jun 10, 2014 at 5:26 AM, Marcello Maggioni <hayarms at gmail.com> wrote:
> > Hello,
> >
> > when LowerSwitch transforms a switch instruction into a tree of ifs it is
> > actually performing a binary search into the various case ranges, to see if
> > the current value falls into one cases range of values.
> >
> > So, if we have a program with something like this:
> >
> > switch (a) {
> > case 0:
> >   do0();
> >   break;
> > case 1:
> >   do1();
> >   break;
> > case 2:
> >   do2();
> >   break;
> > default:
> >   break;
> > }
> >
> > the code produced is something like this:
> >
> >   if (a < 1) {
> >     if (a == 0) {
> >       do0();
> >     }
> >   } else {
> >     if (a < 2) {
> >       if (a == 1) {
> >         do1();
> >       }
> >     } else {
> >       if (a == 2) {
> >         do2();
> >       }
> >     }
> >   }
> >
> > This code is inefficient because the check (a == 1) to execute do1() is not
> > needed.
> >
> > The reason is that because we already checked that (a >= 1) initially by
> > checking that also  (a < 2) we basically already inferred that (a == 1)
> > without the need of an extra basic block spawned to check if actually (a ==
> > 1).
> >
> > The proposed patch addresses this problem by keeping track of already
> > checked bounds in the LowerSwitch algorithm, so that when the time arrives
> > to produce a Leaf Block that checks the equality with the case value / range
> > the algorithm can decide if that block is really needed depending on the
> > already checked bounds .
> >
> > For example, the above with "a = 1" would work like this:
> >
> > the bounds start as LB: NONE , UB: NONE
> > as (a < 1) is emitted the bounds for the else path become LB: 1 UB: NONE.
> > This happens because by failing the test (a < 1) we know that the value "a"
> > cannot be smaller than 1 if we enter the else branch.
> > After the emitting the check (a < 2) the bounds in the if branch become LB:
> > 1 UB: 1. This is because by checking that "a" is smaller than 2 then the
> > upper bound becomes 2 - 1 = 1.
> >
> > When is time to emit the leaf block for "case 1:" we notice that 1 is
> > squeezed exactly in between the LB and UB, which means that if we arrived to
> > that block there is no need to emit a block that checks if (a == 1).
> >
> >
> > Please let me know if I did something wrong here and if the reasoning is
> > correct :)
> >
> > The proposed patch also sets as XFAIL the test:
> > "test/Transforms/LowerSwitch/feature.ll".
> > This test checks that the emitted lowered CFG is exactly the one expected by
> > the previous algorithm without this optimization. Of course this test starts
> > failing when this optimization is added. I added another test that is the
> > same, but is updated with the new expected CFG.
> >
> > Don't know what is the best thing to do with the original "feature.ll" test
> > in the case this patch gets accepted. Should we remove it or keep it as a
> > reference?
> 
> Most likely we should just change the original to test the new
> behavior, and not introduce a new test. If the original test doesn't
> test all cases of the new behavior and can't be reasonably made to do
> so, we could introduce a new test for those new cases (while keeping
> the old one covering the existing cases, but updated to verify the new
> optimization you've implemented is functioning)
> 
> - David
> 
> >
> > Cheers,
> > Marcello
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> 
> 
> <optimized_lower_switch_v2.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140610/c0672575/attachment.html>