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
1256

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
82

Why?

101

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
82

That's just a leftover. Removed.

101

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
96

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
2735–2737

Typo (me -> make)?

6233

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?

6264–6266

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
96

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
2735–2737

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

6233

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
101

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
101

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
72

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.

96

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
72

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.

96

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
72

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
96

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

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++.

96

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}.

101

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.

104

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
326

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

879–880

should assert that Scalable is false

1829

IRBuilder has a CreateVectorSplat that takes an ElementCount

2147

assert not scalable

2294–2295

assert not scalable

2494

assert not scalable

2829

assert not scalable

3285

assert not scalable

3752

assert not scalable

3993–3994

assert not scalable

5525–5526

assert not scalable

5712–5713

assert not scalable

5941

assert not scalable

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.