[PATCH] D24511: [LV] Process pointer IVs with PHINodes in collectLoopUniforms

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5398
@@ +5397,3 @@
+      auto UsersAreMemAccesses = all_of(Ptr->users(), [&](User *U) -> bool {
+        return isa<LoadInst>(U) || isa<StoreInst>(U);
+      });
----------------
mkuper wrote:
> mssimpso wrote:
> > mkuper wrote:
> > > Do we need to check that if U is a store, then Ptr is actually the pointer operand of the store, not the value?
> > > 
> > > I'm thinking about:
> > > store i32* %p, i32** %q,
> > > Where both %p and %q are consecutive.
> > Yes, nice catch! I'll update the patch. We want all users of the pointer to be memory accesses, where the pointer is the pointer operand. This is all we consider when checking for scalarization.
> > 
> > We already know Ptr is the pointer operand of one memory access, but it could be used as the value operand of another. Something like:
> > 
> > ```
> > %x = load i32* %p
> > store i32* %p, i32** %q
> > ```
> > 
> > %p can't remain uniform because we need to store all it's corresponding values to memory. %q remains uniform if the other conditions are met (the store is not scalarized, etc.).
> > 
> Yes, this is exactly what I meant.
Great, I'll add another test case for this.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5466
@@ +5465,3 @@
+      return I == IndUpdate || !TheLoop->contains(I) || Worklist.count(I) ||
+             isVectorizedMemAccessUse(I, Ind);
+    });
----------------
mkuper wrote:
> mssimpso wrote:
> > mkuper wrote:
> > > The only thing that changed here, functionally, is the addition of isVectorizedMemAccessUse right?
> > > The rest is cleanup?
> > That's right. I'm happy to save the cleanup for a separate patch if you prefer.
> My personal preference is to do it the other way around - NFC cleanups go in first, and then you can put up the functional patch (post-cleanup) for review. This kills two birds with one stone:
> 1) We don't have to revert cleanups if the functional patch turns out to be broken.
> 2) Reviewing the functional patch is easier. :-)
> 
> (If you think the cleanup itself is complex enough to deserve review, then I'd prefer two separate reviews, but, again, cleanup first.)
Sounds good. I'll go ahead with the cleanup before updating the patch here.


https://reviews.llvm.org/D24511