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

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:655-659
@@ -654,7 +654,7 @@
 
 /// \brief If the pointer has a constant stride return it in units of its
 /// element size.  Otherwise return zero.
 ///
 /// Ensure that it does not wrap in the address space, assuming the predicate
 /// associated with \p PSE is true.
 ///
----------------
> It might have been clearer to call this getStrideOfPtr().

I agree, and given that this change touches all calls anyway, let's rename it to make it more clear. How about, getPtrStride()?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1619
@@ -1617,3 +1618,3 @@
     bool IsReadOnlyPtr = false;
-    if (Seen.insert(Ptr).second || !isStridedPtr(PSE, Ptr, TheLoop, Strides)) {
+    if (Seen.insert(Ptr).second || !isStridedPtr(PSE, Ptr, Strides)) {
       ++NumReads;
----------------
This use of isStridedPtr also needs a loop check. If the pointer is strided w.r.t. an outer loop only, then it is not consecutive.

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:79
@@ -79,3 +78,3 @@
     // the dependence distance.
-    if (isStridedPtr(PSE, LoadPtr, L) != 1 ||
-        isStridedPtr(PSE, LoadPtr, L) != 1)
+    if (isStridedPtr(PSE, LoadPtr) != 1 ||
+        isStridedPtr(PSE, LoadPtr) != 1)
----------------
Loop check here. If a pointer is strided only w.r.t. an outer loop, it is not unit stride here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4918-4925
@@ -4917,5 +4917,10 @@
     Value *Ptr = LI ? LI->getPointerOperand() : SI->getPointerOperand();
-    int Stride = isStridedPtr(PSE, Ptr, TheLoop, Strides);
-
+    const SCEV *PtrScev = replaceSymbolicStrideSCEV(PSE, Strides, Ptr);
+    
+    // Consider Stride to be zero unless Ptr have constant stride over TheLoop.
+    int Stride = 0; 
+    if (!PSE.getSE()->isLoopInvariant(PtrScev, TheLoop))
+      Stride = isStridedPtr(PSE, Ptr, Strides);
+  
     // The factor of the corresponding interleave group.
     unsigned Factor = std::abs(Stride);
----------------
Maybe we should have an extra overload of the function that takes a loop to check? I think that's cleaner than forcing the caller to repeat PSE.getSE()->isLoopInvariant in each case.


http://reviews.llvm.org/D17268