[PATCH] D23889: [LV] Scalarize instructions marked scalar after vectorization

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Would it make sense to commit the different changes here separately?

That is:
Patch 1: collectLoopUniforms()
Patch 2: The parts that only emit instructions for the low lane of uniforms.
Patch 3: vectorizeBlockInLoop()

If I'm not mistaken both (1) and (2) should be good independently. (1) because it will make other things that care about uniformity slightly more correct, and (2) because it may help some instructions we already scalarize today (although I'm not sure we actually have such cases now. So maybe 2+3 should go in together).


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2257
@@ -2246,1 +2256,3 @@
 
+  auto scalarUserIsUniform = [&](User *U) -> bool {
+    auto *I = cast<Instruction>(U);
----------------
Why "scalar"?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2259
@@ +2258,3 @@
+    auto *I = cast<Instruction>(U);
+    if (!OrigLoop->contains(I))
+      return true;
----------------
Why not

```
return (!OrigLoop->contains(I) || !Legal->isScalarAfterVectorization(I)
        || Legal->isUniformAfterVectorization(I))
```
?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2272
@@ +2271,3 @@
+  if (Legal->isUniformAfterVectorization(cast<Instruction>(EntryVal)) ||
+      all_of(EntryVal->users(), scalarUserIsUniform))
+    Lanes = 1;
----------------
Why do we need this?
Wouldn't we already mark EntryVal as uniform in collectLoopUniforms(), if all its users are uniform / consecutive pointers?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4550
@@ +4549,3 @@
+
+    // Scalarize instructions that should remain scalar after vectorization.
+    if (!(isa<BranchInst>(&I) || isa<PHINode>(&I)) &&
----------------
This scares me a bit. :-)
As I said on the PR, I definitely think we should do it, but it still scares me. Perhaps add a bit more testing?

Regardless, there's (at least?) one more special case I see below - DbgInfoIntrinsic. 


https://reviews.llvm.org/D23889