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

[Adam Nemet via llvm-commits]

anemet added a comment.

Matt, I also had some comments from earlier that I've never sent out.  This was about the future direction of get{Scalar,Vector}Value:

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

> > 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