[PATCH] D30200: [SLP] Fix for PR31880: shuffle and vectorize repeated scalar ops on extracted elements

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Thanks, Alexey.
Some fairly minor comments inline.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:194
+static std::pair<bool, TargetTransformInfo::ShuffleKind>
+isBlendOrShuffle(ArrayRef<Value *> VL) {
+  auto *EI0 = dyn_cast<ExtractElementInst>(VL[0]);
----------------
I think the name is a bit weird. If it's a blend, it's definitely a shuffle. What this really does is determine whether we're extracting from at most 2 sources, right? Maybe give a name that indicates this?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:197
+  if (!EI0)
+    return {false, TargetTransformInfo::SK_Alternate};
+  unsigned Size = EI0->getVectorOperandType()->getVectorNumElements();
----------------
I'm not a fan of using SK_Alternate here. Can you use Optional<>?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:201
+  BitVector UsedElements2(Size);
+  BitVector UndefElements(Size);
+  Value *Vec1 = nullptr;
----------------
Maybe define all 3 on one line? (If you think this is better, feel free to leave as is).


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:205
+  for (auto *V : VL) {
+    auto *EI = dyn_cast<ExtractElementInst>(V);
+    // Allow only extractelement instructions, which users are going to be
----------------
I think you're guaranteed that all members of a VL are the same instruction type, so you can just cast<> here, and avoid the check below.
(Please verify I'm right, though. :-) )


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:241
+  // have blending of 2 vectors.
+  return {!UsedElements1.any(), Vec2
+                                    ? TargetTransformInfo::SK_PermuteTwoSrc
----------------
I think if the order of extracts and inserts just happens to be the same, we'll overestimate the cost. But that's fine for now. Could you check, and if we indeed overestimate, add a FIXME?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:241
+  // have blending of 2 vectors.
+  return {!UsedElements1.any(), Vec2
+                                    ? TargetTransformInfo::SK_PermuteTwoSrc
----------------
mkuper wrote:
> I think if the order of extracts and inserts just happens to be the same, we'll overestimate the cost. But that's fine for now. Could you check, and if we indeed overestimate, add a FIXME?
When can !UsedElements1.any() be true?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1752
     }
+    bool IsBlendOrShuffle;
+    TargetTransformInfo::ShuffleKind ShuffleKind;
----------------
Maybe check whether VL[0] is an extract here? It would make it clearer that the code below is relevant only for extracts, as opposed to checking internally.


https://reviews.llvm.org/D30200