[PATCH] InstCombine: scalarize vector phis

[Nadav Rotem]

> 
> I’m not sure that I follow. The proposed transformation does the same, it creates only one cast. You can see this in the provided test.

In the current transformation the old vector cast goes away because its only user is removed.  In the new transformation that you are proposing you are adding a new cast instruction, but the old vector cast still stays in place. So, after your transformation we will have one more cast operation.

>  
> >> I did not read the code but from a quick peek I noticed that the docs are not capitalized and punctuated properly and the variable names are funny, and you left some code commented out and there are extra braces, etc. 
>  
> I made some corrections.
>  


+Instruction *InstCombiner::scalarizePHI(ExtractElementInst &EI, PHINode *PN) {
+  // Verify that it has only 2 uses. If so, it's known at this point that one
+  // operand is PHI and the other is an extractelement node.
+  if (PN->hasNUses(2)) {
+    // Find the PHI user that is not the extractelement node.

Please use early-exits. There is no need to indent all of the code in the function if we can simply return null. 

+    Value::use_iterator iu = PN->use_begin();
+    Instruction *PHIUser = dyn_cast<Instruction>(*iu);
+    if (PHIUser == cast<Instruction>(&EI)) {
+      PHIUser = cast<Instruction>(*(++iu));
+    }

No need for braces around the PHIUser …   Also, can you add comment that explains this code ?


+    // Verify that this PHI user has one use, which is the PHI itself,
+    // and that it is a binary operation which is cheap to scalarize.
+    if (PHIUser->hasOneUse() && PHIUser->use_back() == PN &&
+      (isa<BinaryOperator>(PHIUser)) &&
+      CheapToScalarize(PHIUser, true)) {

Early exit. 


Except for these comments it looks good to me. 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130417/185c7d1c/attachment.html>