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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

LG with nits
Thanks



================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:89
+
+  // Match a scalar binop with extracted vector operands:
+  // bo (extelt X, C0), (extelt Y, C1)
----------------
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.


================
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;
----------------
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.


================
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();
----------------
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)`?


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

https://reviews.llvm.org/D74495