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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added reviewers: Ayal, gilr.
mkuper added a comment.

Thanks, Alexey.
Could you explain why this is better than just changing the main iteration to go backwards? Wouldn't it solve the same problem, without adding the distinction between different kinds of root nodes?

(Adding Ayal and Gil to this one too.)



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4476
+    if (!I || !I->hasOneUse() ||
+        !findBuildVector(I, BuildVector, BuildVectorOpds))
       return false;
----------------
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.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4494
     InsertValueInst *I = dyn_cast<InsertValueInst>(V);
-    if (!I || !findBuildAggregate(I, BuildVector, BuildVectorOpds))
+    if (!I || !I->hasOneUse() ||
+        !findBuildAggregate(I, BuildVector, BuildVectorOpds))
----------------
Ah, I see, we already had unbounded recursion here. This is kind of unfortunate. :-\
I guess one of us can change this in a separate change-list, but I still wouldn't introduce another case.


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


================
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
----------------
I'm a bit confused. Why is this a good time to call ProcessInstructions()?


================
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);
----------------
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.


https://reviews.llvm.org/D29826