[PATCH] InstCombine: scalarize vector phis

[Shemer, Anat]

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.

The removed check is that the extract has only one use. The check that the cast has only one use remains and after its only use, the extract, is removed, it's eliminated as well.

Fixed patch attached.

Regards, Anat


From: Nadav Rotem [mailto:nrotem at apple.com]
Sent: Wednesday, April 17, 2013 18:58
To: Shemer, Anat
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] InstCombine: scalarize vector phis


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.

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130418/084e02bc/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: InstCombineVectorPHI2.diff
Type: application/octet-stream
Size: 5779 bytes
Desc: InstCombineVectorPHI2.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130418/084e02bc/attachment.obj>