[PATCH] D29449: [SLP] Generalization of vectorization of CmpInst operands, NFC.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked an inline comment as done.
ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4806
     // Try to vectorize trees that start at compare instructions.
-    if (CmpInst *CI = dyn_cast<CmpInst>(it)) {
-      if (tryToVectorizePair(CI->getOperand(0), CI->getOperand(1), R)) {
+    if (auto *BI = dyn_cast<BranchInst>(it)) {
+      if (!BI->isConditional())
----------------
mkuper wrote:
> The code is not equivalent.
> 
> 1) A cmp doesn't have to feed a branch. You can have two icmps feeding an or, for instance, with the i1 being used by a branch. Why would we want to avoid vectorizing those? Or do we already fail to vectorize when the icmp feeds anything but a branch?
> 2) On the flip side, you can have a branch that is fed by an "or (icmp, icmp)", or any other source of i1. I assume this will get filtered out in tryToVectorize() but it'd be good to have a test.
> 
> In any case, the comment above needs to be updated.
You're not quite correct about it.
1. Yes, of course. But this situation will be handled in `vectorizeRootInstruction()` function. It calls `canBeVectorized()` function that traverses all suboperations of the initial instruction (up to `RecursionMaxDepth` tree height). So actually, the current version of the code does the same work twice: the first time when we perform top-to-bottom analysis of all instruction in `vectorizeChainsInBlock()` function and the second time when we perform bottom-to-top analysis in `vectorizeRootInstruction()` function.
The first top-to-bottom analysis breaks vectorization in some cases (for example, for future min/max reductions and maybe in some cases for binops too).
2. Your assumption is correct, this situation is handled by `tryToVectorize()` function. 
And we have a test for these (or similar) situations in `Transforms/SLPVectorizer/X86/compare-reduce.ll`, `Transforms/SLPVectorizer/X86/horizontal-list.ll` and `Transforms/SLPVectorizer/X86/in-tree-user.ll` tests already.
I'll update the comment


Repository:
  rL LLVM

https://reviews.llvm.org/D29449