[PATCH] D80323: [SVE] Eliminate calls to default-false VectorType::get() from Clang

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D80323#2049374 <https://reviews.llvm.org/D80323#2049374>, @ctetreau wrote:

> In D80323#2048457 <https://reviews.llvm.org/D80323#2048457>, @rjmccall wrote:
>
> > I'm sympathetic to wanting to get rid of the boolean flag, but this is a really invasive change for pretty minimal benefit.  Why not leave `VectorType::get` as meaning a non-scalable vector type and come up with a different method name to get a scalable vector?
>
>
> As it stands today, if you call VectorType::get(Ty, N, false), you will get an instance of FixedVectorType, via a base VectorType pointer. Currently, you can call getNumElements() on a base VectorType and it will work, but the ultimate endgame is that getNumElements is going to be removed from base VectorType. This change makes it so that you don't have to cast your VectorType object to FixedVectorType everywhere.
>
> The overall architecture is that there is a derived type for each sort of vector. VectorType is the base class, and the two derived vector types are FixedVectorType and ScalableVectorType. I suppose I could have named them something like BaseVectorType, VectorType(actually fixed width vector type), and ScalableVectorType, but I feel that this is a worse solution because it's not obvious by the name what a "VectorType" is. Additionally, naming the types this would have broken all code that previously worked for scalable vectors. It's not obvious to me which naming scheme would have resulted in less changes (changes to rename fixed vectors to FixedVectorType vs changes needed to rename all generic code to BaseVectorType and to fix code for scalable vectors), but I think the consistency in the naming scheme justifies the path I chose.


Yeah, I understand what you're trying to do.  If I had to guess, I'd say the vast majority of existing frontend, optimizer, and backend code does *not* work with scalable vectors as a free generalization.  The code that *does* work with them needs to at least be audited, and the clearest way of marking that you've done that audit would be to change the types.  So this is breaking a ton of code, including a lot that's not in-tree and which you are therefore not on the hook to update (and it's particularly likely that there's a lot of vector code out-of-tree), as well as setting up the "audit polarity" exactly backwards from what it should be — all just so that you can use the name `VectorType` for the common base type, which is really not much of a win vs. `VectorBaseType` or `AnyVectorType` or `AbstractVectorType` or some similar.

The fact is that people come up with ways to generalize the representation all the time, and it's great for LLVM to take those.  Sometimes generalizations are necessarily disruptive because they're really pointing out a common conflation that's dangerous in some way, but in this case it's purely "additive" and there's really no good reason that any of the existing code that's happily assuming fixed-width vectors should have to change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80323