[llvm-commits] PATCH: instcombine switch on select of constants to br

[Nick Lewycky]

Alistair Lynn wrote:
> Hi-
>
> On 10 Jan 2011, at 10:10, Frits van Bommel wrote:
>> Please don't top-post.
>
> My apologies.
>
>> You're not updating PHI nodes in successors. You should call
>> Succ->removePredecessor(BB) for each successor that is removed.
>
> Done.
>
>> Some edge cases:
>> - The input values are different, and go to different successors.
>> Fold into a conditional branch and keep exactly one copy of both
>> successors.
>> - The input values are different, but their successors are the same.
>> Keep folding this into an unconditional branch, and remove all but one
>> successor. In particular, note that the successor being jumped to will
>> have multiple PHI entries for this predecessor, and only one of those
>> may remain.
>
> I'm confused about that last point - the verifier claims that phi nodes
> should have one and only one entry for each predecessor?

Yes, but you can have the same block as a predecessor multiple times, in 
which case there must be one entry in the phi node for each edge from 
that pred.

And yes, the verifier checks that all incoming values for a given pred 
match for the same pred block. :)

Nick

>> - The input values are equal, or both are caught by the default case.
>> Remove all other successors and keep folding into unconditional
>> branch.
>> - Some successor blocks that are the successor of multiple switch
>> cases which are all removed.
>> - Some successor blocks that are the successor of multiple switch
>> cases, some (but not all) of which are removed.
>>
>> Ideally your test cases should test each of these, with other edges
>> (not from the switch) going to the successors and (used) PHI nodes
>> there.
>
> Okay, tests updated.
>
>> Since all of this may enlarge the code, you may want to factor it out
>> into a static helper function. You could use
>> SimplifyIndirectBrOnSelect() as an example, but note that that one has
>> some different edge cases (for instance, the successors it tries to
>> remove may not actually be present).
>> Maybe you can even share some code between that and your
>> SimplifySwitchOnSelect()? You could probably factor out all of the
>> code SimplifyIndirectBrOnSelect() uses to update successors[1] to a
>> separate helper function; the indirectbr-specific edge cases shouldn't
>> be reachable for a switch so it won't generate incorrect code.
>
> I've factored it out into a separate helper function, but it really didn't
> inflate the code that much. Due to the many differences between
> switch and indirectbr (I don't believe duplicate entries are allowed
> in the successors list for indirectbr), I haven't factored out common
> code between SimplifySwitchOnSelect and
> SimplifyIndirectBrOnSelect, because there really isn't much.
>
> Alistair
>
>
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits