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

[Alistair Lynn]

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?

> - 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch-patch-take-3.diff
Type: application/octet-stream
Size: 5854 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110110/ceffb646/attachment.obj>
-------------- next part --------------