<div>This looks great, please make the changes below and then commit it.</div><div><br></div><div>+  for (BasicBlock::iterator I = Succ->begin(), E = Succ->end(); I != E; ++I) {</div><div>+    PHINode *PHI = dyn_cast<PHINode>(I);</div>


<div>+    if (!PHI) break;</div><div><br></div><div>Actually, this can be simplified further to:</div><div><br></div><div>BasicBlock::iterator I = Succ->begin();</div><div>while (PHINode *PHI = dyn_cast<PHINode>(I++)) { ... }</div>


<div><br></div><div>which is already a pattern in LLVM.</div><div><br></div><div><div>+  for (ForwardingNodesMap::iterator I = ForwardingNodes.begin(),</div><div>+      E = ForwardingNodes.end(); I != E; ++I) {</div></div>


<div><br></div><div>Fix whitespace on the line with the E, it should line up with the F of ForwardingNodesMap, not the ( .</div><div><br></div><div><div>--- /dev/null</div><div>+++ b/test/Transforms/SimplifyCFG/ForwardSwitchConditionToPHI.ll</div>


<div>@@ -0,0 +1,35 @@</div><div>+; RUN: opt < %s -simplifycfg -S | \</div><div>+; RUN:   not grep " switch"</div></div><div><br></div><div>Also add "; PR10131" right after the RUN lines.</div><div>

<br>
</div><div>Thanks for working on this!</div><div><br></div><div>Nick</div><br><div class="gmail_quote">On 12 June 2011 08:47, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Frits,<br>
<br>
Thanks for your comments! Attaching new patch that addresses the issue<br>
you pointed out.<br>
<div><br>
On Sat, Jun 11, 2011 at 6:51 PM, Frits van Bommel <<a href="mailto:fvbommel@gmail.com" target="_blank">fvbommel@gmail.com</a>> wrote:<br>
> I can't comment on whether this transformation is a good idea, but I<br>
> do have some remarks about your code:<br>
><br>
> +  BasicBlock *Succ = Branch->getSuccessor(0);<br>
> +<br>
> +  for (BasicBlock::iterator I = Succ->begin(), E = Succ->end(); I != E; ++I) {<br>
> +    PHINode *PHI = dyn_cast<PHINode>(I);<br>
> +    if (!PHI) continue;<br>
><br>
> All PHI nodes must be at the beginning of the basic block, before any<br>
> other instructions, so you can replace 'continue' with 'break' here.<br>
<br>
</div>Ah, I forgot about that.<br>
<div><br>
><br>
> +    int Idx = PHI->getBasicBlockIndex(BB);<br>
> +    if (Idx < 0) continue;<br>
><br>
> Since PHI is in a successor of BB, the IR would be invalid if the PHI<br>
> didn't have an entry for it. You can replace the check with<br>
>  assert(Idx >= 0 && "PHI has no entry for predecessor?");<br>
<br>
</div>Fixed.<br>
<div><br>
><br>
> +static bool ForwardSwitchConditionToPHI(SwitchInst *SI) {<br>
> +  PHINode *ForwardingNode = NULL;<br>
> +  SmallVector<int, 4> ForwardingNodeIndexes;<br>
> +<br>
> +  for (unsigned I = 1; I < SI->getNumCases(); ++I) { // 0 is the default case.<br>
> +    ConstantInt *CaseValue = SI->getCaseValue(I);<br>
> +    BasicBlock *CaseDest = SI->getSuccessor(I);<br>
> +<br>
> +    int PhiIndex;<br>
> +    PHINode *PHI = FindPHIForConditionForwarding(CaseValue, CaseDest,<br>
> +                                                 &PhiIndex);<br>
> +    if (!PHI) continue;<br>
> +<br>
> +    if (!ForwardingNode)<br>
> +      ForwardingNode = PHI;<br>
> +    else if (PHI != ForwardingNode)<br>
> +      continue;<br>
> +<br>
> +    ForwardingNodeIndexes.push_back(PhiIndex);<br>
> +  }<br>
> +<br>
> +  if (ForwardingNodeIndexes.size() < 2)<br>
> +    return false;<br>
><br>
> I think the use of ForwardingNode here makes this code dependent on<br>
> the order of switch cases: if there are multiple blocks with eligible<br>
> PHIs (in different blocks), but the first one your loop encounters<br>
> happens to only have one switch case pointing at it then no change<br>
> will be made even though the others might still be transformable.<br>
><br>
<br>
</div>You're right. It should be easy enough to find and keep track of all<br>
eligible targets.<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br>