[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:
> > 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?


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

https://reviews.llvm.org/D87772