[PATCH] D87772: [SLP] sort candidates to increase chance of optimal compare reduction

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6944
       }
       i += ReduxWidth;
       ReduxWidth = PowerOf2Floor(NumReducedVals - i);
----------------
spatel wrote:
> ABataev wrote:
> > spatel wrote:
> > > ABataev wrote:
> > > > Maybe, better to change this expression too? The main problem here is that the sliding window has step `ReduxWidth`. Maybe, use `1` here in common case? Also, I don't think sorting is safe here. It is better to keep the original order of the instructions, but pre-select only the matching ones.
> > > I'm not sure what the suggestion is exactly - if we allow any reduction width, will we effectively re-open D59710 and all of its problems (miscompiles, perf loss, etc)?
> > > 
> > > Do you have an idea why sorting is not safe? If I'm seeing it correctly, we have guaranteed that all ops in the reduction are associative and additionally they are all in the same basic block. Therefore, they should be safe to reorder (the reduction itself requires that property?).
> > 1. Ah, yes, I missed that it works only in case of the successful vectorization attempt. What I meant is to add a way to rebuild the `VL` in case if vectorization attempt was unsuccessful. Currently, we just early exit in this case and ignore the remaining part of the reduced values.
> > 2. Yeah, it should be safe. Anyway, could you add a test where the original order of the reduced values is changed by the sorting?
> Sure - I think that is challenging given the current restrictions in this patch because we also have this limitation in buildTree_rec():
>       // Check that all of the compares have the same predicate.
> 
> But there's a loophole for swapped predicate, so I added a test with that possibility. I think this also shows that it is possible to degrade the reduction if the IR is in some non-standard form, but I'm hoping that is not the common case.
> 
> The better solution (assuming this part does not cause trouble) will be to also sort based on the operand values (that's the TODO comment I left in the code).
1. Agreed, might be a good candidate for future work.
2. I think we need to implement both, sorting and improvement for the sliding window. It also might trigger an additional vectorization, though need to be very careful here since it also affects the compile time.


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

https://reviews.llvm.org/D87772