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

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2257
@@ -2246,1 +2256,3 @@
 
+  auto scalarUserIsUniform = [&](User *U) -> bool {
+    auto *I = cast<Instruction>(U);
----------------
mssimpso wrote:
> 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).
Oh, ok, I understand now - this checks "isScalar() implies isUniform()" by computing "!isScalar() || isUniform()".
I read "scalar users" as "pre-vectorization users" not "users that are going to stay scalar", and got confused.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2272
@@ +2271,3 @@
+  if (Legal->isUniformAfterVectorization(cast<Instruction>(EntryVal)) ||
+      all_of(EntryVal->users(), scalarUserIsUniform))
+    Lanes = 1;
----------------
mssimpso wrote:
> 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.
Ah, got it, thanks!

Sentences like "all its scalar users are uniform" still confuse me, because of the terminology we've adopted.
My personal preference is still "uniform" for what we call "uniform scalar" and "multi-scalar" for what we call "non-uniform scalar" - then we could say that we're looking for an IV that has no multi-scalar users. But maybe I just need to get used to the current state. :-)

================
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)) &&
----------------
mssimpso wrote:
> 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?
Hmpf. I think I still prefer handling this at the top - duplicating this check into each case seems silly. 


https://reviews.llvm.org/D23889