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

Matthew Simpson via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 26 07:40:22 PDT 2016


mssimpso added a comment.

Thanks for the quick feedback, Michael! I'll start addressing your comments by first pulling out the changes to collectLoopUniforms in a separate review, and adding some specific tests for it.


================
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:
> 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. :-)
I agree the terminology is a bit confusing. At the very least, I'll add a more detailed comment here.

================
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:
> 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. 
Agreed.


https://reviews.llvm.org/D23889





More information about the llvm-commits mailing list