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

[Frits van Bommel]

On Tue, Jan 11, 2011 at 12:44 AM, Alistair Lynn <arplynn at gmail.com> wrote:
>> You're not updating PHI nodes in successors. You should call
>> Succ->removePredecessor(BB) for each successor that is removed.
>
> Done.

You're not handling duplicate successors correctly. You should call
removePredecessor() once for each time it is removed.

>> 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?

Duplicate predecessors should have duplicate entries. Note that it
doesn't say "for each *unique* predecessor". Each predecessor should
have as many entries in PHIs as the number of times it's block has the
PHI's block as a successor.
It also checks that all entries for the same predecessor must have the
same incoming value.
(In other words: "What Nick said")

Note that the indirectbr code handles "duplicate successors one of
which is kept" by nulling out the variable it used to check for that
successor, so that if it comes across any duplicates of it they are
still removed as successors.
(To make sure it's correct for equal destinations of select operands,
it pre-nulls the "false" destination if it would be the same as the
"true" destination because the unconditional branch used in this case
only has one succcessor)

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

Only one test actually has a PHI node. To make sure you're doing the
right thing, it's best if all successors of the switch you're removing
have a PHI node. That way you can see when you e.g. forget to remove
duplicate entries. (Keeping multiple PHIs alive is easily done by
having the entry block contain a switch too)
Also, IIRC opt will by default run the verifier at the end, so it will
make the test fail if you generate incorrect code like that. However,
it can probably only detect this if there are PHI nodes.

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

indirectbr allows duplicate entries. LangRef.html says "Blocks are
allowed to occur multiple times in the destination list, though this
isn't particularly useful".

> code between SimplifySwitchOnSelect and
> SimplifyIndirectBrOnSelect, because there really isn't much.

Actually, I think the only difference is that in the case of
indirectbr the successors you want to preserve may not actually be
present (and we need to handle this case). This special case will
simply not occur for switches, but using a helper function which
checks for this possibility shouldn't harm anything.

I still think turning the end of SimplifyIndirectBrOnSelect into a
separate helper function is the easiest way to make sure you're not
missing anything.


A stylistic issue: most of the code in your helper function is in an
if (TrueVal && FalseVal). You can reduce indentation and increase
clarity by using
  if (!TrueVal || !FalseVal)
    return false;
instead.