[PATCH] D115811: [SLP]Early exit out of the reordering if shuffled/perfect diamond match found.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1656-1665
+        SmallPtrSet<Value *, 4> UniqueValues;
+        ArrayRef<OperandData> Op0 = OpsVec.front();
+        for (const OperandData &Data : Op0)
+          UniqueValues.insert(Data.V);
+        for (ArrayRef<OperandData> Op : drop_begin(OpsVec, 1)) {
+          if (any_of(Op, [&UniqueValues](const OperandData &Data) {
+                return !UniqueValues.contains(Data.V);
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > ABataev wrote:
> > > > vporpo wrote:
> > > > > ABataev wrote:
> > > > > > ABataev wrote:
> > > > > > > vporpo wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > vporpo wrote:
> > > > > > > > > > Isn't `VLOperands` a better place for this logic? Perhaps a method like: `isDiamondMatch()` ?
> > > > > > > > > > This will also help separate the temporary check `UniqueValues.size() == 2 || !isPowerOf2_32(UniqueValues.size())`. What do you think?
> > > > > > > > > I just thought that we may have this situation after the very first iteration of the reordering, not only initially. 
> > > > > > > > I am not sure I follow why moving this logic to a member method in VLOperands won't work in this case. Could you elaborate a bit on this?
> > > > > > > I have the same problem because this function `reorder()` is a member of `VLOperands` class :)
> > > > > > Or do you suggest transforming this lambda to a member function? If so, I think keeping lambda is better because it does not increase the number of interfaces of the class. If (or when) we'll have several users of this functionality, it can be outlined into a private member function.
> > > > > Yes, I was suggesting moving the loops of this lambda to a function. I understand that this is the only  use so it is not really needed, but if we need this same functionality in the future it will be hard to remember that this code already exists in this lambda. So we will probably end up re-implementing it.
> > > > > 
> > > > > Anyway, regarding your earlier comment, sorry, I still don't understand what issue you are referring to about the first iteration of reordering. Are you referring to the `Pass`es in the for loop? I am confused.
> > > > Yes, to passes in the loop. If the pass failed, we still do some reordering and may end up with the diamond match situation.
> > > OK, but why do you want to avoid it if it happens in a later pass? Is it going to produce a worse reordering?
> > > 
> > > Do you think the diamond case could be handled by adding a new `ReorderingMode::Diamond` that disables reordering for those operand indexes (or perhaps disables reordering completely)? This could be set in the loop under `// Initialize the modes.`, line 1627. This would also fit with the existing design and won't look like a workaround. What do you think?
> > > OK, but why do you want to avoid it if it happens in a later pass? Is it going to produce a worse reordering?
> > I think it may.
> > 
> > > Do you think the diamond case could be handled by adding a new ReorderingMode::Diamond ...
> > 
> > Not sure we can do it. We perform the analysis lane by lane, but here we need to perform the analysis in the orthogonal order - operand by operand.
> > We can implement some deeper analysis, probably, but it requires extra time to understand how to implement it.
> > Plus, this is not the analysis but a kind of corner case check. The analysis process is not aware of the diamond match and currently, it is pretty hard to teach it about it, need to change the design completely.
> Are you planning to move the bail-out check before the pass loop then?
> 
> Also I would rename the lambda to something like `SkipReordering` because it is actually looking for corner cases when it should bail-out, it is not looking for cases when it should apply reordering. And please make sure that it is clear from the comment that this is the place where any code related to reordering bail-outs should be placed.
I intentionally moved it into the loop, because even if we failed to reorder all the operands/lanes, some of them still might be reordered and after the failed reordering attempt we still may have diamond match. 
I’ll rename it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115811