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

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

Thanks for the quick feedback, Michael!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5397
@@ +5396,3 @@
+      // True if all users of the pointer operand are memory accesses.
+      auto UsersAreMemAccesses = all_of(Ptr->users(), [&](User *U) -> bool {
+        return isa<LoadInst>(U) || isa<StoreInst>(U);
----------------
mkuper wrote:
> Do we care about bitcasts / GEP chains? (I'm ok with being conservative with this, just making sure it's by choice.)
This is intentionally conservative, and we can probably relax it eventually. This patch was NFC when I tested it on SPEC, by the way.

But note that Ptr should be the actual pointer operand of a memory access (see comment below). This covers the case where we have a GEP that is bitcast, and the bitcast is then used by the memory access. We look through bitcasts in isConsecutivePointer, which calls getGEPInstruction.

For chains of instructions, we have to prove the user uniform first. This already happens to some degree in the expansion phase of the analysis. If GEP1 is only used by GEP2, and GEP2 remains uniform, GEP1 will be marked uniform as well. The same is true for the GEP in the bitcast case above.

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


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5466
@@ +5465,3 @@
+      return I == IndUpdate || !TheLoop->contains(I) || Worklist.count(I) ||
+             isVectorizedMemAccessUse(I, Ind);
+    });
----------------
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.


https://reviews.llvm.org/D24511