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

[Adam Nemet via llvm-commits]

anemet added a comment.

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.

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.

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

Anyhow, sorry about the somewhat random comments, I was just wondering what you guys thought about this.


https://reviews.llvm.org/D23169