[PATCH] D25276: [LoopVectorizer] Interleaved-mem-accesses analysis and getPtrStride

[Dorit Nuzman via llvm-commits]

dorit added a comment.



In https://reviews.llvm.org/D25276#575290, @mssimpso wrote:

> If I understand things correctly now, we're trying to optimize the case where we have a full interleave group where the stride may wrap. So yes I see that we may want ShouldCheckWrap=false, at least initially. I don't think there is any harm in Assume=true for the initial call, though, because we later in Legality (collectLoopUniforms) call isConsecutivePtr for the pointer operand of every memory instruction (with Assume=true). If we don't use Assume=true for a pointer in the interleaved accesses anaysis, we will do so later in the uniformity analysis, so I think we want to do the same thing in both places.


I think we are all in agreement on that!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5648-5650
+      int64_t Stride =
+          getPtrStride(PSE, Ptr, TheLoop, Strides,
+                       /*Assume=*/false, /*ShouldCheckWrap=*/false);
----------------
mssimpso wrote:
> I think this would be a little cleaner if we added a may wrap flag to StrideDescriptor. We would check the wrapping condition initially here and save that in the StrideDescriptor (and in the interleave groups as we build them). Then for removing groups, we could simply check that it is not full and may wrap.
OTOH, I think there’s also something clean about initially avoiding the wrapping question all together…

Also, how aggressive in terms of allowing runtime checks do we want to be when we check the wrapping condition initially ?
If we allow it, that would be the equivalent of calling getPtrStride with Assume=true, ShouldCheckWrap=true; That might mean adding a lot of tests…
If we don’t allow it, we are too conservative.

Maybe I misunderstood exactly what you are proposing, but I think in any case like the approach of light-weight optimistic initial pass over all pointers, and then selectively checking wrapping only where needed.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5872
+        continue;
+      Value *Ptr = getPointerOperand(Member);
+      int Stride = getPtrStride(PSE, Ptr, TheLoop, Strides, /*Assume=*/false,
----------------
sbaranga wrote:
> I think there is another case here when we don't need to check the wrapping: if the group has the first and last pointers, and we know they both don't wrap.
> 
> We should be able to apply this for versioning later and have at most two no-wrap checks per interleaved group.
So, trying to summarize:
1) Initial check with Assume=true, ShouldCheckWrap=false;
2) Revisit only groups with gaps as follows: 
- if first and last elements are accessed: skip further checking.
- otherwise – check the first and last pointers of the group with Assume=false (for now), ShouldCheckWrap=true;

Silviu, is that what you meant?



================
Comment at: llvm/test/Transforms/LoopVectorize/interleaved-accesses-2.ll:16
+; with proper threshold checks.
+; XFAIL: *
+
----------------
sbaranga wrote:
> mssimpso wrote:
> > I would prefer that we not XFAIL a new test. Unless Silviu disagrees with letting Assume=true, I think it would be better to check the actual behavior while keeping the FIXME.
> > 
> > Would we be missing a test though? If Assume=true here, I don't think we will be checking the case that we actually remove a group that is may wrap?
> I think having Assume=true for the initial check should be fine.
Right; 

Silviu, the example you gave earlier for a strided access that requires Assume=true: How would you turn it into a test that doesn't fail  vectorization on "SCEV could not compute the loop exit count"...? 


https://reviews.llvm.org/D25276