[PATCH] D24557: [Loop Vectorizer] Simplified GEP cloning. NFC.

[Adam Nemet via llvm-commits]

anemet added a comment.

In https://reviews.llvm.org/D24557#543837, @mkuper wrote:

> Ugh. Ignore my last comment, this is wrong.
>
> We have *almost* fixed what "uniform" means in the vectorizer (thanks again, Matt), but not quite.
>
> (a) LVL::isUniform() and LAI::isUniform() are basically equivalent to isLoopInvariant(). They imply the value is equal on all lanes.


We should really remove this guy.  It's totally confusing now.

> (b) LVL::isUniformAfterVectorization() refers to what @anemet is talking about - it's a property of how the value is used, specifically that we only use the lane 0 value.

> 

> So:

>  @anemet: this is already phrased in terms of loop invariance.


Sure, what I meant was that at some point in this discussion, we started formulating the assert in terms of uniformity vs. loop-invariance which I found confusing.

> @delena: I no longer agree with your example.

> 

> If we have:

> 

>   %foo = getelementptr inbounds i8, i8* %ptr, i64 %ind

> 

> 

> And %foo is consecutive, then one of the following holds:

> 

> 1. %ptr is loop-invariant (uniform), and %ind is consecutive.

> 2. %ptr itself is consecutive, and %ind is loop-invariant (uniform).

> 

>   So, I think I understand why the assert fails now. For case 1, where %ind is consecutive, the assert will pass, because %ind is the "induction operand" (that is, it's in the "right place" in the GEP). For case 2, where %ptr is consecutive, the assert may fail, because %ptr has to be consecutive, but doesn't really have to be the IV.

> 

>   This also possibly explains why the assert started failing after you improved consecutivity detection. If we have something like this:

> 

>   ``` %p = getelementptr inbounds i8, i8* %iv, i64 0 %q = getelementptr inbounds i8, i8* %p, i64 1

> 

>   ``` Then as long as only %p is considered consecutive, everything is fine, but if %q is also considered consecutive, the assert should fail.

> 

>   Does this make sense?


Makes sense to me.  Sounds like the assert should state that one of the operands needs to be a loop-variant uniform while the others all loop-invariant.  Agreed?

I would still like @delena to confirm that this is indeed what was happening!


Repository:
  rL LLVM

https://reviews.llvm.org/D24557