[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Hal Finkel]

----- Original Message -----
> From: "Eric Christopher" <echristo at gmail.com>
> To: "Marcello Maggioni" <hayarms at gmail.com>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Tuesday, June 10, 2014 1:28:44 PM
> Subject: Re: [PATCH] Improving LowerSwitch behavior for contiguous ranges
> 
> On Tue, Jun 10, 2014 at 11:24 AM, Marcello Maggioni
> <hayarms at gmail.com> wrote:
> > Hello,
> >
> > thank you for your feedback guys!
> >
> > Answers to everybody:
> >
> > - Pete: Thanks for that, seems like clang-format-diff doesn't
> > understand
> > DEBUG() macros ... :P
> 
> Might want to file a bug and let Daniel Jasper know :)
> 
> > - Philip: I removed as much whitespace changes as I could find
> > - Eric: I changed the comment in head to switchConvert to explain
> > the new
> > Upper and LowerBound arguments (is that what you meant?)
> >
> 
> Yep. Though I have to admit that it'd be nice if we had a range
> propagation pass that would clean this up rather than special casing.
> :\

Doesn't LVI do range propagation?

 -Hal

> 
> -eric
> 
> > Cheers,
> > Marcello
> >
> >
> > 2014-06-10 18:14 GMT+01:00 Eric Christopher <echristo at gmail.com>:
> >
> >> While you're in there updating the documentation for switchConvert
> >> would be good as well.
> >>
> >> -eric
> >>
> >> 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?
> >> >
> >> > Cheers,
> >> > Marcello
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> >
> >
> >
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory