llvm dev - Aug 2020 - [RFC] Replace `unsigned VF` with `ElementCount VF` [NFCI]

Hi all,

I am working on replacing `unsigned VF` with `ElementCount VF` in the
vectorizer.

I have submitted an RFC patch to fabricator just to show how it looks like:
https://reviews.llvm.org/D85794

Just to be clear. I am not trying to get this patch in as it is, I am just
asking for some feedback on the approach.

The changes in the vectorizer are numerous, but mostly mechanical.

I have explained in the description of the patch the changes, which fall in two
categories:

1. changes outsize the vectorizer, which are needed to be able to query things
like "how many lanes" are in this "vectorization factor", or
to print things to debug streams or remarks.
2. changes in the vectorizer that are needed to guarantee a _non functional
change_, for example asserting that the ElementCount instance being used to
compute a vector reverse cannot be scalable. The code essentially looks the same
as before this patch, other than for the fact that I have added such assertions.

I am aware the changes in LoopVectorizer.cpp might be a lot. I am open to
consider different incremental approaches.
One data point here is that my initial approach was to add the `bool Scalable`
property to the VectorizationFactor class.
However, using ElementCount instead of the pair {unsigned, bool} for dealing
with the number of lanes inside the VectorizationFactor seemed a natural choice
that was worth investigating.

Please let me know what you think!

Kind regards,

Francesco

Hi Francesco,

As I said in the review, the approach looks good to me.

I agree this will make it easier for VPlan to create multiple plans like
that, so that's a positive change. And I can't see a negative side for
the
existing vectoriser, other than a bit more clutter with {VF, false} instead
of just VF. But that's not important.

cheers,
--renato

On Tue, 11 Aug 2020 at 23:58, Francesco Petrogalli via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi all,
>
> I am working on replacing `unsigned VF` with `ElementCount VF` in the
> vectorizer.
>
> I have submitted an RFC patch to fabricator just to show how it looks
> like: https://reviews.llvm.org/D85794
>
> Just to be clear. I am not trying to get this patch in as it is, I am just
> asking for some feedback on the approach.
>
> The changes in the vectorizer are numerous, but mostly mechanical.
>
> I have explained in the description of the patch the changes, which fall
> in two categories:
>
> 1. changes outsize the vectorizer, which are needed to be able to query
> things like "how many lanes" are in this "vectorization
factor", or to
> print things to debug streams or remarks.
> 2. changes in the vectorizer that are needed to guarantee a _non
> functional change_, for example asserting that the ElementCount instance
> being used to compute a vector reverse cannot be scalable. The code
> essentially looks the same as before this patch, other than for the fact
> that I have added such assertions.
>
> I am aware the changes in LoopVectorizer.cpp might be a lot. I am open to
> consider different incremental approaches.
> One data point here is that my initial approach was to add the `bool
> Scalable` property to the VectorizationFactor class.
> However, using ElementCount instead of the pair {unsigned, bool} for
> dealing with the number of lanes inside the VectorizationFactor seemed a
> natural choice that was worth investigating.
>
> Please let me know what you think!
>
> Kind regards,
>
> Francesco
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200814/f848d57e/attachment.html>

Hi, Francesco,

I've taken a look at the patch, and I think that this is a good 
approach. One thing that I want to point out here is that the recipe 
mentioned in the patch description, including:

  * VF == 1replaced withVF.isScalar().
  * VF > 1andVF >=2replaced withVF.isVector().
  * VF <=1is replaced withVF.isZero() || VF.isScalar().

appears to me to be a nice readability enhancement to the code 
independent of anything else. There might be a lot of changes, but they 
seem mostly mechanical, and they seem to make the code more readable, so 
they pay for themselves in that regard.

Thanks again,

Hal


On 8/11/20 5:58 PM, Francesco Petrogalli via llvm-dev
wrote:
> Hi all,
>
> I am working on replacing `unsigned VF` with `ElementCount VF` in the
vectorizer.
>
> I have submitted an RFC patch to fabricator just to show how it looks like:
https://reviews.llvm.org/D85794
>
> Just to be clear. I am not trying to get this patch in as it is, I am just
asking for some feedback on the approach.
>
> The changes in the vectorizer are numerous, but mostly mechanical.
>
> I have explained in the description of the patch the changes, which fall in
two categories:
>
> 1. changes outsize the vectorizer, which are needed to be able to query
things like "how many lanes" are in this "vectorization
factor", or to print things to debug streams or remarks.
> 2. changes in the vectorizer that are needed to guarantee a _non functional
change_, for example asserting that the ElementCount instance being used to
compute a vector reverse cannot be scalable. The code essentially looks the same
as before this patch, other than for the fact that I have added such assertions.
>
> I am aware the changes in LoopVectorizer.cpp might be a lot. I am open to
consider different incremental approaches.
> One data point here is that my initial approach was to add the `bool
Scalable` property to the VectorizationFactor class.
> However, using ElementCount instead of the pair {unsigned, bool} for
dealing with the number of lanes inside the VectorizationFactor seemed a natural
choice that was worth investigating.
>
> Please let me know what you think!
>
> Kind regards,
>
> Francesco
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200820/3e8ee028/attachment-0001.html>