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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:241
+  // have blending of 2 vectors.
+  return {!UsedElements1.any(), Vec2
+                                    ? TargetTransformInfo::SK_PermuteTwoSrc
----------------
ABataev wrote:
> RKSimon wrote:
> > mkuper wrote:
> > > 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.
> > @mkuper is right - SK_Alternate is not a simple blend, it should only match shuffles which alternate between sequential elements from the 2 vectors (e.g. 0, 5, 2, 7). I've no idea why we went for that instead of a general SK_Blend but that's where we are...
> Reworked this part
The condition of the ternary expression is kind of confusing. Maybe unpack it a bit?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:239
+      Vec1 = Vec;
+    else if (!Vec2)
+      Vec2 = Vec;
----------------
Shouldn't this be something like

```
else if (Vec1 != Vec)
```
?


https://reviews.llvm.org/D30200