[llvm-commits] [llvm] r163302 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/switch_create.ll test/Transforms/SimplifyCFG/switch_to_lookup_table.ll

[Chandler Carruth]

On Sat, Sep 8, 2012 at 3:04 PM, Evan Cheng <evan.cheng at apple.com> wrote:

> 3. My biggest concern is all these FIXME's
>
>  // FIXME: Handle unreachable cases.
> +
> +  // FIXME: If the switch is too sparse for a lookup table, perhaps we
> could
> +  // split off a dense part and build a lookup table for that.
> +
> +  // FIXME: If the results are all integers and the lookup table would
> fit in a
> +  // target-legal register, we should store them as a bitmap and use
> shift/mask
> +  // to look up the result.
> +
> +  // FIXME: This creates arrays of GEPs to constant strings, which means
> each
> +  // GEP needs a runtime relocation in PIC code. We should just build one
> big
> +  // string and lookup indices into that.
> +
> +  // Ignore the switch if the number of cases are too small.
> +  // This is similar to the check when building jump tables in
> +  // SelectionDAGBuilder::handleJTSwitchCase.
> +  // FIXME: Determine the best cut-off.
> +  if (SI->getNumCases() < 4)
> +    return false;
>
> Have you look into integrating this optimization into SelectionDAGBuilder
> instead? It seems like it very well could benefit from having target
> information. I am not certain it's a good idea to have switch optimizations
> spread out among different passes.
>
> I'm not opposed to a different approach. For example, move all switch
> related optimizations into codegenprep. I've always been a bit annoyed by
> the way switch is lowered in selectiondag.


Just for perspective I had one other idea of how this type of transform
might be implemented. I was talked out of it by someone, although I don't
remember who, but I think it's still at least useful to think about.

My idea had been to extend the IR to directly represent the concept of this
operation. Currently, we have a 'br' instruction to represent binary
control flow selection, and we have a 'switch' instruction to represent
N-way control flow selection. We also have 'select' to represent binary
value selection, but no instruction to represent N-way value selection. I
suggested adding a new instruction 'selectswitch' (or some better name) to
support selecting over N values based on an integer much the way switch
works.

This would allow us to lower this construct in the SelectionDAGBuilder (or
wherever) trivially to whatever target-specific pattern was most suitable.
Within the IR it would provide a canonical and target independent
representation that captured the conceptual independence -- specifically
that this need not cause a control flow change. The SimplifyCFG pass would
then be a pass to form these instructions much like it will sometimes form
select instructions, etc.

The reason I still somewhat like this approach is that it exposes a fairly
powerful invariant to the IR-level optimizers and it canonicalizes nicely
while allowing targets to lower in whatever way is suitable. We have quite
a few optimizations that are enabled by the presence of select in the IR,
and it seems a shame to loose that when the branch is N-way instead of
2-way.

The reason others argued against this (if I remember correctly...) is that
there is no direct target analog to this construct. We only have 'select'
to model the ability of a target to do a 'cmov'. This has never really sat
well with me:
1) There are IR-level optimizations it enables even for targets which have
no cmov support.
2) Merely simplifying the CFG for every other pass seems directly
beneficial to all the algorithms which scale with the CFG structure.
3) We arguable do have support for an N-way selection: the exact
optimization this pass provides of loading from a table with a particular
index. It doesn't work for things still in-register, but I don't see what
that's terribly relevant.

Anyways, I mostly just wanted to relay this discussion so you could be
aware of it in thinking about what the final structure of this should be.
I'm not hugely opposed to the current approach, or the approach you
describe of doing this in codegenprep or selectiondagbuilder.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120908/f3487b14/attachment.html>