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

[Nick Lewycky]

This looks great, please make the changes below and then commit it.

+  for (BasicBlock::iterator I = Succ->begin(), E = Succ->end(); I != E;
++I) {
+    PHINode *PHI = dyn_cast<PHINode>(I);
+    if (!PHI) break;

Actually, this can be simplified further to:

BasicBlock::iterator I = Succ->begin();
while (PHINode *PHI = dyn_cast<PHINode>(I++)) { ... }

which is already a pattern in LLVM.

+  for (ForwardingNodesMap::iterator I = ForwardingNodes.begin(),
+      E = ForwardingNodes.end(); I != E; ++I) {

Fix whitespace on the line with the E, it should line up with the F of
ForwardingNodesMap, not the ( .

--- /dev/null
+++ b/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll
@@ -0,0 +1,35 @@
+; RUN: opt < %s -simplifycfg -S | \
+; RUN:   not grep " switch"

Also add "; PR10131" right after the RUN lines.

Thanks for working on this!

Nick

On 12 June 2011 08:47, Hans Wennborg <hans at chromium.org> wrote:

> 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.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110617/87ec510a/attachment.html>