Re: [PATCH] D17268: [LAA] Function 'isStridedPtr' returns additional result “Loop *Lp” via function argument and add appropriate checks out of the 'isStridedPtr'.

[Ayal Zaks via llvm-commits]

Ayal added a comment.

Looks ok to me; leaving for Hal or Adam to formally LGTMize.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:655
@@ -654,2 +654,3 @@
 /// \brief Check the stride of the pointer and ensure that it does not wrap in
-/// the address space, assuming \p Preds is true.
+/// the address space, assuming \p Preds is true. Returns via func. argument
+/// \p Loop over which pointer is strided. \p Lp might be nullptr in case of zero
----------------
Preds? (Admittedly not part of your patch)

Suggest to start by describing that the function "Returns the stride of \p Ptr or zero if \p Ptr is not strided". Then add that "\p Lp is an output parameter, expected to be null on entry; if \Ptr is strided across loop L, \Lp is set to L."

If \Ptr is strided across multiple loops (as in PR26314), \Lp is set to the innermost of them, right? 

Alternatively isStridedPtr() can return Lp and set the Stride as an output parameter. That however would require two checks for consecutivity.

It might have been clearer to call this getStrideOfPtr().

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:574
@@ -572,3 +573,3 @@
           (!ShouldCheckStride ||
-           isStridedPtr(PSE, Ptr, TheLoop, StridesMap) == 1)) {
+           (isStridedPtr(PSE, Ptr, Lp, StridesMap) == 1 && Lp != nullptr && Lp->contains(TheLoop)))) {
         // The id of the dependence set.
----------------
"&& Lp != nullptr &&" >> "&& Lp &&"

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:851
@@ -847,7 +850,3 @@
 
-  // The accesss function must stride over the innermost loop.
-  if (Lp != AR->getLoop()) {
-    DEBUG(dbgs() << "LAA: Bad stride - Not striding over innermost loop " <<
-          *Ptr << " SCEV: " << *AR << "\n");
-    return 0;
-  }
+  // Return via func. argument the loop over which access function is strided,
+  // so the appropriate checks could be performed by the caller of isStridedPtr.
----------------
func. >> function

================
Comment at: test/Analysis/LoopAccessAnalysis/stride-vectorization.ll:3
@@ +2,3 @@
+; CHECK-LABEL: Test
+; CHECK: %vector.body
+
----------------
better check for a vector type?


http://reviews.llvm.org/D17268