[llvm-commits] [Patch] Replace switches with lookup tables (PR884)

[Sean Silva]

I really like the idea of this patch, and I think it can have a
significant code-size improvement. One huge improvement I expect is
for constructs like

const char *getStringOf(enum Foo val) {
switch(val) {
case Foo_Bar: return "Bar";
case Foo_Baz: return "Baz";
...
}

not that that code is in the hot patch (I hope).

+  // Maybe we can squeeze the table into an integer.

The small integer shift/mask optimization seems like it could be a
separate patch. The FIXME for using the smallest integer type possible
to pack the bitmask seems like it should be an integral part of the
logic, rather than an afterthought, and doing the small integer
optimization there would allow for a solid focused implementation.
Also, the code for handling bitmap lookup tables is really
intermingled with the regular lookup table code; it seems like this
could be factored better, and a separate patch would naturally
encourage that.

+  // FIXME: If the switch is too dense for a lookup table, perhaps we could
+  // split off a dense part and build a lookup table for that.

Do you mean "if the switch is too sparse"?

In the switch lowering code, there is code that does this kind of
analysis (e.g., breaking the cases into groups). I'm not sure how
easy/hard it would be to reuse though.

+  // If the number of cases is too small, don't bother.
+  if (SI->getNumCases() < 4)
+    return false;

eww.... hard coded constant threshold. If this was chosen
experimentally, then I applaud you, but my guess is that it wasn't.
Some details of the experimental setup that chose this would be good
in the former case, and a FIXME begging to run some experiments to
find a good value for this would be good in the latter case.

+  // Chicken out for large spreads to avoid any overflows below.
+  if (RangeSpread.zextOrSelf(64).ugt(1 << 20))
+    return false;

+  if (SI->getNumCases() * 10 < TableSize * 4) {
+    TableTooSparse = true;

These choices seem pretty arbitrary as well.

--Sean Silva

On Mon, Sep 3, 2012 at 7:21 AM, Hans Wennborg <hans at chromium.org> wrote:
> Hi all,
>
> The attached patch adds code to SimplifyCFG so that it attempts to
> turn switch instructions into loads from lookup tables. It works on
> switches that are only used to initialize one or more phi nodes in a
> common successor basic block, for example:
>
> int f(int x) {
>   switch (x) {
>   case 0: return 5;
>   case 1: return 4;
>   case 2: return -2;
>   case 5: return 7;
>   case 6: return 9;
>   default: return 42;
> }
>
> This speeds up the code by removing the hard-to-predict jump. It also
> reduces code size by removing the code for the jump targets.
>
> With the attached micro benchmark, which hits the non-default cases
> about half the time, I get 43% speed-up with my patch.
>
> As a bonus, if the lookup table is small enough to fit in a
> target-valid integer, we can replace the load with shift and mask
> operations. This seems to perform about the same as with the table,
> but I imagine it is much faster for cases when the lookup-table isn't
> in cache. It also reduces code size in some situations (e.g. 32 bools
> would take 32 bytes in a lookup table, but only 4 bytes in an int.)
>
> When I first discussed this on IRC, it was suggested that maybe the
> best place to do this is during the lowering to the selection DAG. In
> my patch I've done it on the IR level because it seemed to fit in well
> in SimplifyCFG, it was easy to do here, and I was hoping that this
> could exploit other IR passes as well. For example, it would be nice
> if two nested switches could become two table lookups. That currently
> doesn't happen, but maybe it could in the future if we moved code
> downwards out of switches more aggressively. If you think having this
> in the lowering code is better, I can try to move it there instead.
>
> Please take a look.
>
> Thanks,
> Hans
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>