[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 reviewer: delena.
mkuper 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]);
----------------
ABataev wrote:
> 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?
I think that makes sense. It's not ideal, but I don't have any good ideas either. "IsUpToTwoVectorShuffle" is... meh.


================
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
----------------
ABataev wrote:
> 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.
Ah, I see, ok.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:241
+  // have blending of 2 vectors.
+  return {!UsedElements1.any(), Vec2
+                                    ? TargetTransformInfo::SK_PermuteTwoSrc
----------------
ABataev wrote:
> 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`.
Oh, I'm sorry, I misread &= as |=.

This still looks wrong, though. The way I understand it, right now SK_Alternate (which usually, for x86, maps to a blend - but the two are not equivalent) has very specific requirements for the mask. See isAlternateVectorMask() in CostModel.cpp. I don't think your code matches that.

Adding Elena  who should probably know better than I do.


https://reviews.llvm.org/D30200