[PATCH] D29826: [SLP] General improvements of SLP vectorization process.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4476
+    if (!I || !I->hasOneUse() ||
+        !findBuildVector(I, BuildVector, BuildVectorOpds))
       return false;
----------------
mkuper wrote:
> We generally try not to have unbounded recursion. I guess you could add a bound here, but I think it would be better to just rewrite this iteratively.
Actually, it is s full copy of `findBuildAggregate` function. Ok, will rewrite it iteratively.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4747
+  SmallDenseSet<Instruction *, 4> KeyNodes;
+  auto &&ProcessInstructions = [this, &PostProcessInstructions, BB,
+                                &R]() -> bool {
----------------
mkuper wrote:
> Maybe split this out? It looks fairly large for a lambda. Especially since it captures "this".
Agree, will add functions instead of this long lambda.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4800
+      if (it->use_empty() && KeyNodes.count(&*it) > 0 &&
+          ProcessInstructions()) {
+        // We would like to start over since some instructions are deleted
----------------
mkuper wrote:
> I'm a bit confused. Why is this a good time to call ProcessInstructions()?
Because we ran into a key node, that was analyzed already, but we found some CmpInst, InsertElement or InsertValue instructions that were not processed yet (we're restarting the vectorization for the whole BB each time we succeded in one of our vectorization attempt). For example, it might be some new instructions, that were inserted and that may be optimized too.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4832
+    // with ignored return value, store.
+    if (it->use_empty() && (it->getType()->isVoidTy() || isa<CallInst>(it))) {
+      KeyNodes.insert(&*it);
----------------
mkuper wrote:
> I'm not sure this is the right condition.
> On the one hand, I'm tempted to just say that any instruction which has use_empty() would be a good candidate - but I'm not sure whether we want to vectorize dead code. Then again, there aren't really too many cases of side-effecting non-void instructions. The only other case I can think of off the top of my head is InvokeInst.
Yes, I thought about InvokeInst, but thought that it is derived from CallInst. Will add it.
At least, we're not worse in this case than were before.


https://reviews.llvm.org/D29826