Page MenuHomePhabricator

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

Authored by ctetreau on May 20 2020, 2:31 PM.

Diff Detail

Event Timeline

ctetreau created this revision.May 20 2020, 2:31 PM
Herald added a project: Restricted Project. · View Herald Transcript

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?

fpetrogalli accepted this revision.May 21 2020, 9:40 AM

LGTM, it would be great if you could address the two comments I added, before submitting.

clang/lib/CodeGen/CGBuiltin.cpp
10775–10777

auto?

clang/lib/CodeGen/CGExprScalar.cpp
4670

auto *

This revision is now accepted and ready to land.May 21 2020, 9:40 AM

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?

Sorry @rjmccall , I saw your comments only after approving this patch. I didn't mean to enforce my views over yours.

@ctetreau , please make sure you address @rjmccall comment before submitting.

Kind regards,

Francesco

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.

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

ctetreau updated this revision to Diff 265542.May 21 2020, 10:49 AM

address code review issues

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.

rjmccall requested changes to this revision.May 21 2020, 11:17 AM

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.

This revision now requires changes to proceed.May 21 2020, 11:17 AM

@rjmccall Given the outcome of the call yesterday, may I merge this patch?

rjmccall accepted this revision.May 29 2020, 12:35 PM

Yeah, LGTM.

This revision is now accepted and ready to land.May 29 2020, 12:35 PM
This revision was automatically updated to reflect the committed changes.