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

[Adam Nemet via llvm-commits]

anemet added a comment.

In http://reviews.llvm.org/D17268#421474, @roman.shirokiy wrote:

> Thanks for the review!
>
> I'll try to describe proposed change using the following example (PR26314). Up-to-date Clang (cfe/trunk 268486) is unable to vectorize this case.
>
>   #define Z 32
>   typedef struct s {
>   	int v1[Z];
>   	int v2[Z];
>   	int v3[Z][Z];
>   } s;
>  
>   void slow_function (s* const obj) {
>       for (int j=0; j<Z; j++) {
>           for (int k=0; k<Z; k++) {
>               int x = obj->v1[k] + obj->v2[j];
>               obj->v3[j][k] += x;
>           }
>       }
>   }
>  
>
>
> Vectorization in this case is denied due to false result "AccessAnalysis::canCheckPtrAtRT" function.
>
> Two calls of "AccessAnalysis::canCheckPtrAtRT" are possible during loop analysis, and check of pointers strides is performed only with the second call, which occurs only in particular case of failed memory dependency check of pointers with non-constant distance between them.
>
> "AccessAnalysis::canCheckPtrAtRT" returns false on the example because if-condition below is false due to "isStridedPtr" zero result on inner-loop-invariant pointer check w.r.t. inner loop "TheLoop".
>
>   if (hasComputableBounds(PSE, StridesMap, Ptr, TheLoop) &&
>       // When we run after a failing dependency check we have to make sure
>       // we don't have wrapping pointers.
>       (!ShouldCheckStride ||
>        isStridedPtr(PSE, Ptr, TheLoop, StridesMap) == 1)) {...}
>   else {
>     DEBUG(dbgs() << "LAA: Can't find bounds for ptr:" << *Ptr << '\n');
>     CanDoRT = false;
>   }
>   
>
> After http://reviews.llvm.org/rL264243 "hasComputableBounds" returns true in case of loop-invariant pointer (like "v2[j]" in the example), but, if I understood it correctly, current "isStridedPtr" will always return zero in this case because there is a check for equality of "TheLoop" (inner) and the loop which is obtained with SCEV of "Ptr" (outer loop for "v2[j]").
>
> When it comes to stride check in "AccessAnalysis::canCheckPtrAtRT" we have as given that there was successful previous run (without the stride check), so either no RT check is needed at all or the pointers are already found suitable for RT check. If we only have to make sure that none of the pointers are wrapping then unit stride of the pointer w.r.t. any loop might be enough?


Ah, thanks for writing this up.  For some reason, I thought that we were failing somewhere else during the second phase (when we prove independence with RT checks only).

I still don't think that tweaking isStridedPtr to prove non-wrapping for more cases is the right approach.  If the address is not a recurrence (e.g. just a global address) that is also trivially non-wrapping but would fail early in getPtrStride.

How about leaving isStridedPtr/getPtrStride unchanged and creating a new helper isNoWrap (similar to isNoWrapAddRec).  isNoWrap would essentially be isLoopInvariant(...) || getPtrStride(...) == 1.

That would also satisfy Hal's request to hide this behind a call.

What do you think?

> 

> 

> In http://reviews.llvm.org/D17268#420503, @anemet wrote:

> 

> > I also think that if you want to rename the function you should probably do that in a separate patch.  BTW, I agree with the renaming.

> 

> 

> Should I create separate diff for review?


No need to.  You can just commit that part right away and then rebase this patch on top of that.


http://reviews.llvm.org/D17268