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

[Hans Wennborg]

Thanks for the review! New patch attached.

 - Hans

On Tue, Sep 4, 2012 at 3:53 AM, Sean Silva <silvas at purdue.edu> wrote:
>> 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.
>
> Yes, a cross reference would be good here, along with the FIXME :)

Done.

>>> +  if (SI->getNumCases() * 10 < TableSize * 4) {
>>> +    TableTooSparse = true;
>>>
>>
>> This one is also to match the jump table logic (SelectionDAGBuilder.cpp:2104).
>
> Same for this one. Cross reference + FIXME.

Done.

> +  // Chicken out for large spreads to avoid any overflows below.
> +  if (RangeSpread.zextOrSelf(64).ugt(UINT64_MAX / 10))
> +    return false;
>
> On my initial review, I honestly didn't see the connection between
> this and the *10 a couple lines down (I guess I interpreted "below" as
> a bit farther down). You should factor this and the surrounding code
> into a static function isTooSparse(), and do like:

Looking at this again I realized that the TableSize doesn't get
multiplied by 10, but by 4, below.

I'm not sure moving it to another function would help much. It should
both compute the table size, check sparseness, and check for the
overflow, and return these three differently.

I've tried to reorganize the code a little to hopefully make it clearer instead.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch_to_lookup_table3.patch
Type: application/octet-stream
Size: 18806 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120904/993355e8/attachment.obj>