[llvm-commits] [Patch] Use bitmaps for switch lookup tables that fit in registers

[Hans Wennborg]

Thanks for the comments! New version attached, split into two patches.

 - Hans

On Thu, Sep 20, 2012 at 2:08 AM, Sean Silva <silvas at purdue.edu> wrote:
>> The patch also refactors the code to put the table-building
>> functionality in a separate class. I'm happy to break that out into a
>> separate patch if preferred.
>
> Please. It's extremely hard to pick out just the new bitmask-table
> building code or just the refactoring to a class. It's OK to send two
> patches in an email (just make sure they have numbered prefixes so
> that it is unambiguous in what order to apply them).

Done.

> A couple comments:
>
> +            const SmallVector<std::pair<ConstantInt*, Constant*>, 4>& Results,
>
> With a name like "Results" this could easily be confused (read: it
> confused me) with being an out parameter.

RIght, let's call it Values instead.

> +/// SwitchLookupTable - This class represents a lookup table that can
> be used to
> +/// replace a switch.
> +class SwitchLookupTable {
>
> Please wrap this class in an anonymous namespace as per the coding
> standards <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces>.

Done.

> +    std::vector<Constant*> TableContents(TableSize);
>
> Why vector and not SmallVector?

You're right, SmallVector will work fine here.

> +    SwitchLookupTable Table;
> +    Table.Init(Mod, TableSize, MinCaseVal, ResultLists[PHI],
> +               DefaultResults[PHI], TD);
> [...]
> +    Value *Result = Table.BuildLookup(TableIndex, Builder);
>
> What's the point of having Init? It just seems like an error-prone
> constructor that might not be called. Maybe better to factor it as one
> class that "analyzes" the switch, and another that does the building;
> that way, the type system enforces things that the current approach
> leaves up to the programmer.

Right, the Init function looks pretty unnecessary here. Removed.



> On Wed, Sep 19, 2012 at 1:32 PM, Hans Wennborg <hans at chromium.org> wrote:
>> Hi all,
>>
>> The attached batch updates the switch-to-lookup table transformation
>> in SimplifyCFG to put the lookup tables in a bitmap when they would
>> fit in a register on the target. This saves some space, and it also
>> allows for building tables that would otherwise be too sparse.
>>
>> One interesting case that this hits is example 7 from
>> http://blog.regehr.org/archives/320. We currently generate good code
>> for this when lowering the switch to the selection DAG: we build a
>> bitmask to decide whether to jump to one block or the other. My patch
>> will result in the same bitmask, but it removes the need for the jump,
>> as the return value can just be retrieved from the mask.
>>
>> This was part of my original lookup table patch [1], but it was
>> suggested in review that we should do it separately.
>>
>> I tried this on a bootstrap build of Clang, and it reduced the binary
>> size with 8328 bytes (0.03%).
>>
>> The patch also refactors the code to put the table-building
>> functionality in a separate class. I'm happy to break that out into a
>> separate patch if preferred.
>>
>> Please take a look.
>>
>> Thanks,
>> Hans
>>
>>  [1]. http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120903/149605.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 001_switch_lookup_table.patch
Type: application/octet-stream
Size: 9686 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120920/5d4cd8be/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 002_switch_bitmap.patch
Type: application/octet-stream
Size: 14940 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120920/5d4cd8be/attachment-0001.obj>