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

[Sean Silva]

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

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.


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

+    std::vector<Constant*> TableContents(TableSize);

Why vector and not SmallVector?

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

--Sean Silva

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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>