[llvm-commits] [Patch] SimplifyCFG: Forward switch condition value to phi node

[Hans Wennborg]

Hi Frits,

Thanks for your comments! Attaching new patch that addresses the issue
you pointed out.

On Sat, Jun 11, 2011 at 6:51 PM, Frits van Bommel <fvbommel at gmail.com> wrote:
> I can't comment on whether this transformation is a good idea, but I
> do have some remarks about your code:
>
> +  BasicBlock *Succ = Branch->getSuccessor(0);
> +
> +  for (BasicBlock::iterator I = Succ->begin(), E = Succ->end(); I != E; ++I) {
> +    PHINode *PHI = dyn_cast<PHINode>(I);
> +    if (!PHI) continue;
>
> All PHI nodes must be at the beginning of the basic block, before any
> other instructions, so you can replace 'continue' with 'break' here.

Ah, I forgot about that.

>
> +    int Idx = PHI->getBasicBlockIndex(BB);
> +    if (Idx < 0) continue;
>
> Since PHI is in a successor of BB, the IR would be invalid if the PHI
> didn't have an entry for it. You can replace the check with
>  assert(Idx >= 0 && "PHI has no entry for predecessor?");

Fixed.

>
> +static bool ForwardSwitchConditionToPHI(SwitchInst *SI) {
> +  PHINode *ForwardingNode = NULL;
> +  SmallVector<int, 4> ForwardingNodeIndexes;
> +
> +  for (unsigned I = 1; I < SI->getNumCases(); ++I) { // 0 is the default case.
> +    ConstantInt *CaseValue = SI->getCaseValue(I);
> +    BasicBlock *CaseDest = SI->getSuccessor(I);
> +
> +    int PhiIndex;
> +    PHINode *PHI = FindPHIForConditionForwarding(CaseValue, CaseDest,
> +                                                 &PhiIndex);
> +    if (!PHI) continue;
> +
> +    if (!ForwardingNode)
> +      ForwardingNode = PHI;
> +    else if (PHI != ForwardingNode)
> +      continue;
> +
> +    ForwardingNodeIndexes.push_back(PhiIndex);
> +  }
> +
> +  if (ForwardingNodeIndexes.size() < 2)
> +    return false;
>
> I think the use of ForwardingNode here makes this code dependent on
> the order of switch cases: if there are multiple blocks with eligible
> PHIs (in different blocks), but the first one your loop encounters
> happens to only have one switch case pointing at it then no change
> will be made even though the others might still be transformable.
>

You're right. It should be easy enough to find and keep track of all
eligible targets.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: forward-switch-condition-to-phi2.patch
Type: text/x-patch
Size: 5508 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110612/723bcfa4/attachment.bin>