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

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

In https://reviews.llvm.org/D23889#525581, @mkuper wrote:

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


Right, that's what I was thinking. I wanted to included everything here in the review (at least initially) for context. We can split it up exactly as you suggested. Regarding an independent patch 2, it should be NFC post-instcombine. Pre-instcombine, we would at least no longer generate unnecessary steps for uniform IVs.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2257
@@ -2246,1 +2256,3 @@
 
+  auto scalarUserIsUniform = [&](User *U) -> bool {
+    auto *I = cast<Instruction>(U);
----------------
mkuper wrote:
> Why "scalar"?
This probably makes more sense with the response below. We're checking to see if all the scalar users of the IV we are scalarizing are also uniform. We can scalarize an IV if it has both vector and scalar users (ending up with two versions).

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2259
@@ +2258,3 @@
+    auto *I = cast<Instruction>(U);
+    if (!OrigLoop->contains(I))
+      return true;
----------------
mkuper wrote:
> Why not
> 
> ```
> return (!OrigLoop->contains(I) || !Legal->isScalarAfterVectorization(I)
>         || Legal->isUniformAfterVectorization(I))
> ```
> ?
That should work!

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2272
@@ +2271,3 @@
+  if (Legal->isUniformAfterVectorization(cast<Instruction>(EntryVal)) ||
+      all_of(EntryVal->users(), scalarUserIsUniform))
+    Lanes = 1;
----------------
mkuper wrote:
> Why do we need this?
> Wouldn't we already mark EntryVal as uniform in collectLoopUniforms(), if all its users are uniform / consecutive pointers?
This is for the cases in which we both want a vector and a scalar version of an IV. We scalarize an IV that's not marked scalar after vectorization when it has at least one scalar user. For such an IV, if all it's scalar users are also uniform, we will only ever need the first lane of the scalar IV.

================
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)) &&
----------------
mkuper wrote:
> hfinkel wrote:
> > mkuper wrote:
> > > 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. 
> > > As I said on the PR, I definitely think we should do it, but it still scares me. Perhaps add a bit more testing?
> > 
> > Which PR?
> My bad, not a PR, meant "on the other review". :-)
> https://reviews.llvm.org/D23509
Right! Thanks for pointing out the debug intrinsics. Perhaps it would make better sense to handle all of the cases individually, instead all together at the top?


https://reviews.llvm.org/D23889