[PATCH] D78601: [SVE] Do not store a bool for Scalable in VectorType

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

> Nit: the name `ElementQuantity` seems to refer to some exotic property of a `VectorType`, while all it refers to it is the minimum number of lanes of the vector that are available in the vector. To give a direct meaning to the name of the field, I think you should rename it `MinNumberOfLanes`, or `KnownMinNumberOfLanes`, and rename `getElementQuantity` accordingly. Getting the minimum number of lanes is an operation that makes perfectly sense for both Scalable and Fixed Type vectors, so there should be no controversies in defining this method in VectorType and let scalable and fixed vector types inherit it?

I specifically picked this name to keep its meaning as ambiguous as possible. Right now, for all existing vector types, it's a lower bound on the number of elements. Maybe at some point in the future, a new vector type is added where the quantity is not a lower bound. Perhaps this is just yak shaving though. Since the field is private, I don't mind changing it.

I prefer not to add a public function to base VectorType that gets just an unsigned right now. It's certainly a reasonable function, but my concern is that right now we're in a transition period, and most people will want to take the path of least resistance to fix their code. I think we'll see a lot of code that looks like this:

  unsigned ActualNumElements = cast<VectorType>(Ty)->getNumElements();

... become:

  unsigned ActualNumElements = cast<VectorType>(Ty)->getMinNumElements();

... instead of something more robust.

For now, I'll rename the field, and make it protected so that the derived vectors can directly return it instead of needing a getter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78601/new/

https://reviews.llvm.org/D78601