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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
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]);
----------------
mkuper wrote:
> 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?
Renamed it to `isShuffle`. Is this ok?


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


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


================
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
----------------
mkuper wrote:
> 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. :-) )
No, we're not guaranteed. This function works when we found out that this is gathering of elements. In this case, we have no information that all instructions are of the same type.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:241
+  // have blending of 2 vectors.
+  return {!UsedElements1.any(), Vec2
+                                    ? TargetTransformInfo::SK_PermuteTwoSrc
----------------
mkuper wrote:
> 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?
Yes, we're overestimated. Reworked it a bit. If `UsedElements1.any()` is `false`, it is `SK_Alternate` (i.e. blending, because we're not crossing lanes in `extractelement` instructions for different vectors), otherwise it is still estimated as `SK_PermuteTwoSrc` or `SK_PermuteSingleSrc`.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1752
     }
+    bool IsBlendOrShuffle;
+    TargetTransformInfo::ShuffleKind ShuffleKind;
----------------
mkuper wrote:
> 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.
Ok, will do.


https://reviews.llvm.org/D30200