This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Do not store a bool for Scalable in VectorType
ClosedPublic

Authored by ctetreau on Apr 21 2020, 5:03 PM.

Details

Summary
  • Whether or not a vector is scalable is a function of its type. Since

all instances of ScalableVectorType will have true for this value and
all instances of FixedVectorType will have false for this value, there
is no need to store it as a class member.

Diff Detail

Event Timeline

ctetreau created this revision.Apr 21 2020, 5:03 PM
Herald added a project: Restricted Project. · View Herald Transcript

Hi @ctetreau ,

thank you for working on this!

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?

Thank you,

Francesco

llvm/include/llvm/IR/DerivedTypes.h
408

I think it would be great to explain the different meaning here, for both FixedVectorType and ScalableVectorType.

Something like:

/// The element quantity of this vector. The meaning of this value depends
/// the type of vector:
/// For FixedVectorType = <m x ty>, ElementQuantity = m
/// Fox ScalableVectorType = <vscale x m x ty>, ElementQuantity = m

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.

ctetreau updated this revision to Diff 259640.Apr 23 2020, 11:20 AM

address code review issues

Thank you for updating the patch @ctetreau

[...] Maybe at some point in the future, a new vector type is added where the quantity is not a lower bound.

If that's the case, I think that your name is better. Otherwise we will have to find a better name if someone comes up with a different use of the field for some fancy vector type that don't treat it as a "mi num of element". Apologies for the extra work!

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.

Makes totally sense to me, thank you for explaining!

fpetrogalli accepted this revision.Apr 24 2020, 5:27 AM

LGTM, feel free to rename the field and related method back to ElementQuantity`, as I said in my last comment.

Thank you,

Francesco

This revision is now accepted and ready to land.Apr 24 2020, 5:27 AM
ctetreau updated this revision to Diff 259909.Apr 24 2020, 9:29 AM

rename field back to ElementQuantity

I went ahead and renamed the field back to ElementQuantity. The comment should still be fine; if somebody adds a new vector type in the future, the comment won't be incorrect, just incomplete.

fpetrogalli accepted this revision.Apr 24 2020, 9:47 AM

L very GTM, thanks!

No problem, thanks for doing code review!

This revision was automatically updated to reflect the committed changes.