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

[Hans Wennborg]

Thank you very much for the review! New patch attached.

On Mon, Sep 3, 2012 at 4:16 PM, Sean Silva <silvas at purdue.edu> wrote:
> 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).

Yes, that's the kind of code I'm going after. And we have quite a bit
of that in LLVM too (for example, lib/Support/Dwarf.cpp). There are
also lots of functions like this that just return true or false, e.g.
the functions in
http://clang.llvm.org/doxygen/PrintfFormatString_8cpp_source.html#l00568
And LLVM is pretty good in synthesizing switches from if statements,
such as Example 7 on http://blog.regehr.org/archives/767, where I
think the shift/mask optimization comes is nicely.

> +  // 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.

I agree, I'll remove this from the current patch.

> +  // 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"?

Yup, fixed.

> 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.

RIght. I think it should be done in a separate patch, 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.

You got me :) No, I did not run experiments. I did peek at the code in
SelectionDAGBuilder.cpp that builds the jump tables, and that has the
same check (line 2095). I can put in a note about that, or a fixme to
figure out the best cutoff.

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

Yeah, this is arbitrary. Checking that it's <= UINT64_MAX / 10 should
be sufficient. I'll update the patch to do that.

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

This one is also to match the jump table logic (SelectionDAGBuilder.cpp:2104).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch_to_lookup_table2.patch
Type: application/octet-stream
Size: 18527 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120903/a66f05f2/attachment.obj>