Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
LGTM, it would be great if you could address the two comments I added, before submitting.
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
10777–10779 | auto? | |
clang/lib/CodeGen/CGExprScalar.cpp | ||
4681 | auto * |
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.
For your specific proposal, I think it would be very strange if a function with the signature static FixedVectorType *get(Type *, unsigned) were a member of the base VectorType.
If you'd like to know more about the motivation for this work, here is a link to the RFC: http://lists.llvm.org/pipermail/llvm-dev/2020-March/139811.html
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.
I've responded to the llvmdev thread, which I missed before. I would like us to hold off on this line or work until we come to a resolution there.
auto?