Page MenuHomePhabricator

[llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI]
ClosedPublic

Authored by fpetrogalli on Aug 11 2020, 3:37 PM.

Details

Summary

Changes:

  • Change ToVectorTy to deal directly with ElementCount instances.
  • VF == 1 replaced with VF.isScalar().
  • VF > 1 and VF >=2 replaced with VF.isVector().
  • VF <=1 is replaced with VF.isZero() || VF.isScalar().
  • Replaced the uses of llvm::SmallSet<ElementCount, ...> with llvm::SmallSetVector<ElementCount, ...>. This avoids the need of an ordering function for the ElementCount class.
  • Bits and pieces around printing the ElementCount to string streams.

To guarantee that this change is a NFC, VF.Min and asserts are used
in the following places:

  1. When it doesn't make sense to deal with the scalable property, for

example:

a. When computing unrolling factors.
b. When shuffle masks are built for fixed width vector types

In this cases, an
assert(!VF.Scalable && "<mgs>") has been added to make sure we don't
enter coepaths that don't make sense for scalable vectors.

  1. When there is a conscious decision to use FixedVectorType. These

uses of FixedVectorType will likely be removed in favour of
VectorType once the vectorizer is generic enough to deal with both
fixed vector types and scalable vector types.

  1. When dealing with building constants out of the value of VF, for

example when computing the vectorization step, or building vectors
of indices. These operation _make sense_ for scalable vectors too,
but changing the code in these places to be generic and make it work
for scalable vectors is to be submitted in a separate patch, as it is
a functional change.

  1. When building the potential VFs in VPlan. Making the VPlan generic

enough to handle scalable vectorization factors is a functional change
that needs a separate patch. See for example `void
LoopVectorizationPlanner::buildVPlans(unsigned MinVF, unsigned
MaxVF)`.

  1. The class IntrinsicCostAttribute: this class still uses `unsigned

VF` as updating the field to use ElementCount woudl require changes
that could result in changing the behavior of the compiler. Will be done
in a separate patch.

  1. When dealing with user input for forcing the vectorization

factor. In this case, adding support for scalable vectorization is a
functional change that migh require changes at command line.

Note that in some places the idiom

unsigned VF = ...
auto VTy = FixedVectorType::get(ScalarTy, VF)

has been replaced with

ElementCount VF = ...
assert(!VF.Scalable && ...);
auto VTy = VectorType::get(ScalarTy, VF)

The assertion guarantees that the new code is (at least in debug mode)
functionally equivalent to the old version. Notice that this change had been
possible because none of the methods that are specific to FixedVectorType
were used after the instantiation of VTy.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ctetreau added inline comments.Aug 14 2020, 12:45 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6185–6186

assert not scalable

6484–6488

assert not scalable

6712–6713

assert not scalable

llvm/lib/Transforms/Vectorize/VPlan.cpp
303–304

assert not scalable

llvm/lib/Transforms/Vectorize/VPlan.h
154–156

NIT: the message doesn't match the VF being scalable. Should be split out

vkmr added inline comments.Aug 14 2020, 12:57 PM
llvm/include/llvm/Support/TypeSize.h
96

@fpetrogalli, I saw your reply after writing my comment.
My main concern was just the condition in isVector(), which in the original draft was return (Scalable || Min > 1;, that would incorrectly report {0, true} to be a vector, (but not {0, false}.
As I mentioned in the other comment, for this patch I would prefer a consistent behavior for fixed and scalable vectors where it makes sense. We can remove the asserts for "invalid values: zero can not be scalable". Also isZero() works for both, so I am not too worried.

ctetreau added inline comments.Aug 14 2020, 1:02 PM
llvm/include/llvm/Support/TypeSize.h
101

Thinking on this, I kind of like having two static functions ElementCount::Fixed(unsigned) and ElementCount::Scalable(unsigned), and hiding all other constructors. This will ensure that, in a hypothetical future where some third kind of vector is added, that code that thinks it's correctly constructing a fixed width ElementCount actually is.

My previous comment stands, for the most part, only instances of VectorType should be constructing ElementCounts manually. But if some other code needs to, we should make it easy to do right.

fpetrogalli marked 27 inline comments as done.
fpetrogalli added a subscriber: david-arm.

Thank you again for the review. In the last update I have:

  1. Removed the methods getScalar and getZero of

ElementCount. The constructor is not invoked directly when needed. I
agree with @ctetreau that ideally we should hide all constructor of
ElementCount in terms of using static method getFixed and
getScalable, but I would like to do this in a separate
patch. perhapd, this could be done as a follow up patch to the changes
that @david-arm is implementing in making the members ElementCount
private (see D86065).

  1. I have added all the missing assertions. Thank you for pointing them out, @ctetreau.
  1. I have converted some of the FixedVectorType::get into

VectorType::get. To guarantee that the code is still an NFC, I have
added an assert that checks that the ElementCount instance being
sued to construct the vector type is not scalable.

  1. I have updated the description of the differential review to

address for these changes.

  1. BestVF in VPlan is now of type Optional<ElementCount> so that

we don't need to construc any ElementCount instance with Min = 0.

llvm/include/llvm/Support/TypeSize.h
96

I have removed the constrain in the constructor.

101

I see your point here. If it is OK for you (please confirm), I will create a separate patch that add these two static methods and remove the explicit constructor. For this patch, I have (temporarily) gone for using explicit constructors.

104

I see your point. I have removed getZero and getScalar.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2294–2295

Done, I have also replaced FixedVectorType with VectorType.

fpetrogalli edited the summary of this revision. (Show Details)Aug 17 2020, 2:35 PM

What you have so far seems like a good approach. I'll refain from giving approval as this is still WIP and other people should give it a look. It might be easier to get it in after D86065 and any follow patches to that land anyways.

llvm/include/llvm/Support/TypeSize.h
101

That's fine. It probably makes sense to do this either in D86065 or as a follow up to that patch. The way I see it, this work is about "using ElementCount" and David's is about "improving ElementCount"

@ctetreau - I had made the constructor of ElementCount private in https://reviews.llvm.org/D86120.

I have updated the changes on top of https://reviews.llvm.org/D86120 (no constructors for ElementCount).

fpetrogalli edited the summary of this revision. (Show Details)
fpetrogalli marked an inline comment as done.
fpetrogalli retitled this revision from [WIP][llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI] to [llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI].Aug 18 2020, 9:49 AM

I put a comment on most of them, but just to elaborate: for any assert of the form assert(!SomeElementCount.Scalable && "invalid number of elements");, the issue is that "invalid number of elements" could mean literally anything (is it invalid because there are 4 elements? Who knows). Obviously if you look at the expression being asserted, it narrows it down to just scalable = true being invalid. But it doesn't answer the question "why is it invalid?". Is it actually nonsensical? Is it just not implemented? I may have missed some, but if I did it was an oversight.

Overall, this patch is almost there. We just need to get the bike shed painted the correct color. :)

llvm/include/llvm/Support/TypeSize.h
88

since we allow zero-element scalable element counts now, this assert should be removed

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
181

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

232–235

update this comment

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
322

The assert message is unclear. Change it to something like "scalable vectors not yet supported".

If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

842

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

879–880

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

1196

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

1308

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

1326

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

1332–1334

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

1825

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

1962–1964

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2146–2148

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2294–2295

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2349–2350

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2367

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2397–2398

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2449

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2494

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

2827

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

3284–3285

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

3419

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

3993–3994

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

4318

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

4463

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

4852

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

5341

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

5525–5526

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

5712–5713

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

5940–5942

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

5950

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

5988

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

6071

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

6185–6186

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

6296

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

6482

The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType

6624

NIT: remove /*Min*/, it's unambiguous what this 1 is here.

6957

For BestVF.getValue(), I would use Optional<T>::operator*

fpetrogalli marked 38 inline comments as done.

Thank you for the review, @ctetreau.

Francesco

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
322

I think that this function makes sense also for scalable vectors.

ctetreau accepted this revision.Aug 19 2020, 10:09 AM

Fix that one straggler, then this LGTM.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1196

looks like you missed this one

This revision is now accepted and ready to land.Aug 19 2020, 10:09 AM
fpetrogalli marked an inline comment as done.Aug 19 2020, 12:01 PM

I fixed one assertion message that I missed in last patch update.

Thank you for the approval @ctetreau.

Francesco

rengolin accepted this revision.Aug 19 2020, 12:42 PM

I'm a bit wary of all the new asserts. The vectoriser is reasonable self-contained, and if you check for the scalable flag at entry points, you can assume safety inside.

But I'm not wary enough to go through the list of all possible entry-points. I think if the plan is to start implementing those pieces right away, then it could be fine as it for the time being.

I only have two small comments and LGTM. Thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6297

unnecessary test on !VF.Scalable already checked by assert.

6838

Name overloading, but now with different types. this will cause confusion.

Better to have ElementCount VF = UserVF and then use VF.Min locally.

fpetrogalli marked 2 inline comments as done.

Thank you @rengolin, I have addressed your feedback.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6838

Good point, done. This allowed some extra improvements like replacing !UserVF.Min with UserVF.isZero().

This revision was landed with ongoing or failed builds.Aug 24 2020, 6:40 AM
This revision was automatically updated to reflect the committed changes.