[PATCH] D23169: [LV] Unify vector and scalar maps

[Adam Nemet via llvm-commits]

anemet added a comment.

In https://reviews.llvm.org/D23169#506915, @mssimpso wrote:

> In https://reviews.llvm.org/D23169#506561, @anemet wrote:
>
> > As we're piling on more complexity on the top of WidenMap, I feel like that the completely permissive ValueParts &get() interface makes it really hard to reason about this code.  The new eraseVector API is certainly a warning sign.
>
>
> Right, that is confusing. We use "get" (now getScalar/getVector) to return an existing entry in the map or initialize an empty entry it if it's not already there. We use "getVectorValue" to return an existing entry or construct a non-empty entry (by broadcasting or, now, by inserting scalar elements into a vector). getVector/getScalar should probably just be initializers. For example, I think we should be able to replace all but one instance of getVector with getVectorValue. Then, getVector/getScalar could be renamed to initVector/initScalar, which would assert or noop if we try to initialize a key more than once. What do you think?


That would be very nice.

> > I think the new logic is correct because we first widen definitions before getting to the uses.  So when widening the definition we either create the vector variant or the scalar variant or in some cases of int induction variables both.  And then as we encounter uses, we derive the missing scalar or vector variants from each unless we already have a value for it.  Please correct me if if I am misinterpreting this.  It would also be good to have a high-level overview of these interactions commented somewhere before the class.

> 

> 

> That's right. Sure I'll add some high-level comments.


Thanks.

> > Anyhow, this all seems like the property of WidenMap, so I am wondering how hard it would be to pull getScalarValue and getVectorValue under that class.  (I am also not sure that hiding the class inside InnerLoopVectorizer in a .cpp file is really adding anything but certainly hurts readability as the class is growing.)

> 

> 

> I think moving getVectorValue and getScalarValue into WidenMap might be tricky. The issue is that getVectorValue uses getBroadcastInstrs, which is a member of InnerLoopVectorizer, and hasStride from LoopVectorizationLegality. For getBroadcastInstrs, would we pass an InnerLoopVectorizer* to WidenMap? Alternatively, we could probably make getBroadcastInstrs into a utility (maybe in VectorUtils?). What about hasStride? What do you think?


Yeah, hasStride does *not* seem to belong to WM.  We could have a wrapper of WM's getVectorValue in ILV that just swaps in the speculated stride value before calling WM::getVectorValue.

getBroadcastInstrs on the other hand probably does belong to WM because it's part of the whole picture how values are mapped.  So probably making it stand-alone utility is the way to go.

Anyhow this all is certainly for another patch.  I just wanted to see what you guys thought about moving even more of the mapping responsibility out of ILV to WM.


https://reviews.llvm.org/D23169