This is an archive of the discontinued LLVM Phabricator instance.

[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

fpetrogalli created this revision.Aug 11 2020, 3:37 PM
Herald added a project: Restricted Project. · View Herald Transcript
fpetrogalli requested review of this revision.Aug 11 2020, 3:38 PM
fpetrogalli retitled this revision from [llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI] to [WIP][llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI].Aug 11 2020, 3:59 PM
fpetrogalli edited the summary of this revision. (Show Details)

Hi @fpetrogalli, thanks for this proposal.

We had to do similar things when we applied the LoopVectorizer to RISC-V V-ext. However we didn't use ElementCount so our version looks much less neat than your approach.

I think this direction makes a lot of sense.

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

Can we use VF.isScalar() here as well or I am missing something?

I think this is a good direction, too.

I don't like implicit assumptions or magic constants, especially when the two (bool scalable and int min) are not linearly independent.

The fact that the patch is huge doesn't make much difference when it's mostly mechanical.

There may be some leftovers, so I suggest you do a grep for Min > and variations, to make sure you got all of them.

I have some comments inline but am mostly happy with it.

llvm/include/llvm/Support/TypeSize.h
84

Why?

103

Should we have a getScalable(int Min), getNonScalable(int Min) and getDisabled() too?

There are a number of places you changed from VF to {VF, false} and places you use literal {0, false} that would benefit from having proper names as the static builders. Using them everywhere would make the code safer and clearer to read.

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
232–235

Here, a getDisabled() would make the code clearer...

fpetrogalli marked 3 inline comments as done.Aug 12 2020, 9:58 AM

We had to do similar things when we applied the LoopVectorizer to RISC-V V-ext. However we didn't use ElementCount so our version looks much less neat than your approach.

Out of curiosity, what is your approach? Carrying around a bool Scalable flag?

I think this direction makes a lot of sense.

Thanks!

llvm/include/llvm/Support/TypeSize.h
84

That's just a leftover. Removed.

103

Should we have a getScalable(int Min), getNonScalable(int Min) and getDisabled() too?

I'd rather not, for the following reasons. Please let me know if you disagree.

  1. getScalable(int Min), getNonScalable(int Min) for replacing uses when using {VF, false}

The instances of {VF, false} are there just because I want to keep this a non-functional change. Those uses are mostly in places were VPlan is computing the possible ElementCount to evaluate. Once we extend VPlan to add also the scalable ECs, we will (likely) just add an extra level of loop iteration on the two values false/true of scalable, and use the ElementCount constructor directly. Once that is done, the two static methods you propose will become obsolete.

I agree that looking at the code with {VF, false} may be confusing. Would you prefer me to explicitly call the constructor instead of using an initializer list, so that we know we are dealing with ElementCount?

So, I see two approaches, let me know which one is the best in your opinion:

a. call the constructor explicitly instead of the initializer list
b. have a static method getFixed or getNonScalable (I prefer the former) that we use but mark it as deprecated as we shouldn't really be using it once the vectorizer doesn't care anymore about scalable and non scalable ECs.

  1. getDisabled

I am happy to add a method for the value of 'zero", but calling it getDisabled is misleading because ther eis no concept of Enabled/Disabled in ElementCount. I'd prefer to call it getZero. I am going this route for this change, let me know if you disagree, will figure something out.

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
232–235

I used getZero, with a comment saying that 0 EC means vectorization is disabled.

fpetrogalli marked 2 inline comments as done.Aug 12 2020, 10:00 AM

I think this is a good direction, too.

I don't like implicit assumptions or magic constants, especially when the two (bool scalable and int min) are not linearly independent.

As mentioned in one of the inline replies, I'd rather avoid creating getScalable and getNonScalable methods in ElementCount. I have explained why, and proposed two alternative approaches. Let me know what you think.

There may be some leftovers, so I suggest you do a grep for Min > and variations, to make sure you got all of them.

On it! Thank you for pointing this out.

I have some comments inline but am mostly happy with it.

Thanks!

Thank you @rengolin and @rogfer01 for the review.

I have applied all feedback, but I have proposed an alternative approach to the one suggested by @rengolin in one of the comments, for static methods like ElementCount::get[Non]Scalable.

It might be worth if you folks also reply to the RFC in the maling list to say that you are happy with this proposal.

I have applied all feedback, but I have proposed an alternative approach to the one suggested by @rengolin in one of the comments, for static methods like ElementCount::get[Non]Scalable.

If the suggested methods will become unused once we start iterating over the scalable component, then perhaps we can live with those {VF, false} things. Calling the constructor makes more explicit what is being constructed so I think it is better than just the initializer-list (at expense of some extra verbosity).

ElementCount::getDisabled does not seem ideal as it is unclear what a "disabled" ElementCount means. ElementCount::getZero looks to me slightly better as long we stay within the domain of "counting things".

Out of curiosity, what is your approach? Carrying around a bool Scalable flag?

We keep some state around that tells us if we're in a scalable mode. But this doesn't work if we want to be able to vectorize both for fixed size or scalable at the same time for the same target. Not sure if you're considering this scenario (e.g. Advanced SIMD + SVE-256)

vkmr added a comment.EditedAug 13 2020, 6:08 AM

Thanks @fpetrogalli for this proposal! I agree with @rogfer01 and @rengolin in that this approach makes a lot of sense.
I have added a few comments inline. Overall looks good.

llvm/include/llvm/Support/TypeSize.h
98

Unless there is something explicitly preventing from constructing a {0, true} ElementCount, this could potentially hide bugs by returning true for this case. Constraining ElementCount constructor is probably a better way.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2730–2732

Typo (me -> make)?

6212

Missing message for assert.
I believe this assert falls under point 4 (When building the potential VFs in VPlan. Making the VPlan generic) in your description of where it would be a functional change to support scalable vectors, right?

6243–6245

Missing message in assert.

fpetrogalli marked 5 inline comments as done.Aug 13 2020, 1:43 PM
fpetrogalli added inline comments.
llvm/include/llvm/Support/TypeSize.h
98

Ah - good point! I have added an assertion in the constructor and made the condition here more strict to that {0, true} is not considered a vector.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2730–2732

Yep, thank you. I have also added an assert as this is a good place where we have to make a conscious choice for enabling scalable vectorization

6212

Yes, you are correct. I have added this message:

assert(!VF.Scalable && "the cost model is not yet implememented for scalable vectorization");
fpetrogalli marked 3 inline comments as done.

Thank you for the reviews.

@vkmr, I have addressed your comments.

I have replaced the initializer lists used for bnuilding ECs instances
with the explicit constructor. The reasoning that led to this makes me
thing that maybe we shoudl make Min and scalable private in the
ElementCount class... but that definitely for a separate patch.

Out of curiosity, what is your approach? Carrying around a bool Scalable flag?

We keep some state around that tells us if we're in a scalable mode. But this doesn't work if we want to be able to vectorize both for fixed size or scalable at the same time for the same target. Not sure if you're considering this scenario (e.g. Advanced SIMD + SVE-256)

I don't see why we should exclude this scenario. Better to keep the possibilities open for now.

rengolin added inline comments.Aug 14 2020, 2:10 AM
llvm/include/llvm/Support/TypeSize.h
103

Once we extend VPlan to add also the scalable ECs, we will (likely) just add an extra level of loop iteration on the two values false/true of scalable, and use the ElementCount constructor directly. Once that is done, the two static methods you propose will become obsolete.

I see what you mean, makes sense.

I agree that looking at the code with {VF, false} may be confusing. Would you prefer me to explicitly call the constructor instead of using an initializer list, so that we know we are dealing with ElementCount?

No, init-list constants are usually more readable than explicit constructor calls. The point was not init-list vs c-tor, but constant vs named call, which won't work long term, as you explained above.

  1. getDisabled

I am happy to add a method for the value of 'zero", but calling it getDisabled is misleading because ther eis no concept of Enabled/Disabled in ElementCount. I'd prefer to call it getZero. I am going this route for this change, let me know if you disagree, will figure something out.

LGTM. Thanks!

fpetrogalli added inline comments.Aug 14 2020, 6:47 AM
llvm/include/llvm/Support/TypeSize.h
103

No, init-list constants are usually more readable than explicit constructor calls.

In the last update of the patch (before your comment) I have replaced the init-list with explicit constructor calls. I just want to make sure you are happy with this. I am am fan of init-list in general, but in this specific case I prefer the constructor, as it make it explicit that we are building something that is used to count the number of elements.

ctetreau added inline comments.
llvm/include/llvm/Support/TypeSize.h
74

I'm uncomfortable with having this comparitor. is {true, 4} > {false, 4}? Depends on the machine. I would personally prefer a named function that can be passed to the comparator parameter of containers. I understand that this makes the code uglier, so if you get significant pushback it may be worth caving on this issue.

98

Constraining the constructor doesn't help with the case where a valid ElementCount is constructed, and then Min set to zero later on.

Why exactly is {true, 0} forbidden? forall N, N * 0 = 0 after all.

@fpetrogalli I haven't fully reviewed it yet, but assuming it does what it says on the tin I think this is a great approach.

fpetrogalli marked an inline comment as done.Aug 14 2020, 11:53 AM
fpetrogalli added inline comments.
llvm/include/llvm/Support/TypeSize.h
74

I think what you say makes sense. I had to introduce a comparison operator because llvm::SmallSet is being used in VPlan.

An alternative approach would be to remove the small set and have a hash function that could be used for std::unordered_set. I actually prefer this latter approach as it avoid introducing the concept of ordering ElementCount, which is kind of formally wrong as you have pointed out.

98

Mathematically you are right. ElementCount is a monomial of grade 0 (a*X^0) or 1 (a*X^1). It doesn't really matter if you are multiplying by 0*X^0 or 0*X^1, both values will null the result of the operation independently of the EC in input.

I think that @vkmr was mostly concerned about potential bugs that can happen when we check if we are working in scalable mode (so Scalable == true) but we forget to check that we have actually a non zero value by checking Min > 0. It is very unlikely to happen, but I guess there is some argument for choosing this approach.

All in all, I'd rather avoid {0, true}, at list in debug mode, just to catch things that might end up weird if we end up producing this value.

fpetrogalli added inline comments.Aug 14 2020, 12:06 PM
llvm/include/llvm/Support/TypeSize.h
74

Ah, llvm::SmallSetVector! I think I am going to try that first, so we don't need both ordering function and the STL.

I have removed the ordering function of ElementCount. It required replacing the uses of llvm::SmallSet<ElementCount,...> with llvm::SmallSetVector<ElementCount, ...>.

Commit message has been updated accordingly.

fpetrogalli edited the summary of this revision. (Show Details)Aug 14 2020, 12:22 PM
fpetrogalli marked an inline comment as done.Aug 14 2020, 12:30 PM
vkmr added inline comments.Aug 14 2020, 12:41 PM
llvm/include/llvm/Support/TypeSize.h
98

@ctetreau I see your point about constraining the constructor for scalable vectors; ElementCount is a counter and 0 is a valid count. Also, however one defines a "vector with 0 elements", it would be the same for fixed and scalar vectors. So, IF the constructor allows constructing fixed vectors of 0 length, it should also allow scalable vectors of 0 length.

The perspective behind not allowing to have Min = 0 (for both fixed and scalable vectors) is that ElementCount's only purpose is to count the number of elements in a vector, and a vector with no elements is illegal vector. But that discussion would be out of scope of this patch.

The updated condition for isVector() still makes sense though.

I understand that this is WIP, but I went ahead and pointed out several missing asserts. I assume the real patch will be based on this at least to some extent.

llvm/include/llvm/Support/TypeSize.h
74

Honestly, I think it's reasonable to want to put an ElementCount in an ordered map or set. If it's just one place, it might be an easier sell to use an explicit named comparator.

I think it's really unfortunate that operator< is overloaded in this way in C++.

98

So you're telling me that there's no good reason to construct a {0, true}, so as a debugging aid you're asserting that it doesn't happen? Why is {0, false} OK? I could see some valid code doing something like:

ElementCount EC = {std::min(someQuantity, someOtherQuantity), true};
if (!EC.isVector())
   return false;

Sure, I could delay construction of the ElementCount, but there's no good reason that I should have to do that.

Really though, as long as the Min and Scalable fields are public, I don't think we should try to preserve any invariants for them. They should either be made private (probably outside the scope of what you are trying to do), or its functionality should be expected to work for any combination of {Min, Scalable}.

103

My thinking is that people shouldn't be manually constructing ElementCount objects very often. FixedVectorType and ScalableVectorType both have static getters that construct instances using only a number, and vectors have a method to get an ElementCount. I would think code that generically produces vectors that may either be fixed width or scalable from the ground up would not be common.

As for having a deprecated getFixed(), I'd like to point out that we have build bots that build with -werror, so deprecating methods with the intention of using them in upstream isn't going to work out.

106

I don't think we should add a getScalar and getZero, because it would be easy to assume these work for scalable vectors end up writing a bunch of bad code. Sure it's an easy fix, but best to just avoid the opportunity for misuse.

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

perhaps this should just be an llvm::Optional<ElementCount>?

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

this can just become VectorType::get(Ty, VF)

874–875

should assert that Scalable is false

1824

IRBuilder has a CreateVectorSplat that takes an ElementCount

2142

assert not scalable

2289–2290

assert not scalable

2489

assert not scalable

2824

assert not scalable

3279

assert not scalable

3746

assert not scalable

3972–3973

assert not scalable

5504–5505

assert not scalable

5691–5692

assert not scalable

5920

assert not scalable

ctetreau added inline comments.Aug 14 2020, 12:45 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6164–6165

assert not scalable

6463–6467

assert not scalable

6691–6692

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
98

@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
103

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
98

I have removed the constrain in the constructor.

103

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.

106

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

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2289–2290

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
103

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
90

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
317

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

837

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

874–875

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

1191

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

1303

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

1321

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

1327–1329

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

1820

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

1957–1959

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

2141–2143

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

2289–2290

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

2344–2345

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

2362

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

2392–2393

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

2444

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

2489

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

2822

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

3278–3279

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

3413

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

3972–3973

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

4297

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

4442

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

4831

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

5320

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

5504–5505

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

5691–5692

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

5919–5921

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

5929

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

5967

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

6050

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

6164–6165

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

6275

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

6461

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

6603

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

6935

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
317

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
1191

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
6276

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

6817

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
6817

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.