[PATCH] D74495: [VectorCombine] try to form vector binop to eliminate an extract element

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 5 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:89
+
+  // Match a scalar binop with extracted vector operands:
+  // bo (extelt X, C0), (extelt Y, C1)
----------------
lebedev.ri wrote:
> Hm, and if there is only one extract, and the other operand is constant,
> we consider that vectorizing the constant in some way will always be costlier?
> Just thinking out loud, not for this patch.
This would imply that the vector binop is cheaper than the scalar binop since we would have the same extract either way. As I mentioned in an earlier comment, I haven't found that case yet on x86, but it's possible it exists for some binop on some subtarget.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:118-119
+  // vector total because those instructions will not be eliminated.
+  if (C0 == C1) {
+    if (X != Y) {
+      int ScalarCost = Extract0Cost + Extract1Cost + ScalarBOCost;
----------------
lebedev.ri wrote:
> If we are extracting the same lane, the cost should be identical right?
> I wonder if it would be more readable to assert that,
> do `int ExtractCost = Extract0Cost;` and operate on it here.
Yes, this is a leftover of starting the patch on the more general case. I'll change it.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:132
+      // unoptimized form with identical values.
+      bool HasUseTax = Ext0 == Ext1 ? Ext0->hasNUsesOrMore(3)
+                                    : !Ext0->hasOneUse() || !Ext1->hasOneUse();
----------------
lebedev.ri wrote:
> I'd think it would be more obvious to do `!Ext0->hasNUses(2)` instead.
> It can't have less than two uses - there are two uses in our entry binop.
> And if there are more than two uses then those are The extra uses.
> 
> Otherwise, why do we check `hasOneUse()` instead of `hasNUsesOrMore(2)`?
I agree that your suggestion is better for readability. Will change. Thanks for the detailed review!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74495/new/

https://reviews.llvm.org/D74495