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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6944
       }
       i += ReduxWidth;
       ReduxWidth = PowerOf2Floor(NumReducedVals - i);
----------------
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?).


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

https://reviews.llvm.org/D87772