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

[Frits van Bommel]

On 11 June 2011 18:25, Hans Wennborg <hans at chromium.org> wrote:
> The attached patch is an attempt to fix PR10103.
>
> In cases such as the attached test, where the case value for a switch
> destination is used in a phi node that follows the destination, it
> might be better to replace that value with the condition value of the
> switch, so that more blocks can be folded away with
> TryToSimplifyUncondBranchFromEmptyBlock because there are less
> conflicts in the phi node.
>
> I'm not familiar with the optimizations code, so please take a look
> and let me know if this is a good idea or not. Your comments are most
> welcome.

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.

+    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?");

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