llvm dev - May 2020 - [llvm-commits@lists.llvm.org: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses]

Bringing this up on llvm-dev for more general attention.

The problem here is two fold:

(1) Reuse of enumeration values is just a major no-go.
(2) I'm not sure why the existing vector types had to be killed
completely.

But something clearly has to be done here. This majorly affects e.g.
Mesa.

Joerg

----- Forwarded message from Joerg Sonnenberger via llvm-commits
<llvm-commits at lists.llvm.org> -----

Date: Sun, 10 May 2020 02:34:12 +0200
From: Joerg Sonnenberger via llvm-commits <llvm-commits at lists.llvm.org>
To: Christopher Tetreault <ctetreau at quicinc.com>, Christopher Tetreault
<llvmlistbot at llvm.org>
Cc: llvm-commits at lists.llvm.org
Subject: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses
Reply-To: Joerg Sonnenberger <joerg at bec.de>

On Wed, Apr 22, 2020 at 08:59:20AM -0700, Christopher Tetreault via llvm-commits
wrote:
> 
> Author: Christopher Tetreault
> Date: 2020-04-22T08:59:01-07:00
> New Revision: 2dea3f129878e929e5d1f00b91a622eb1ec8be4e
> 
> URL:
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e
> DIFF:
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e.diff
> 
> LOG: [SVE] Add new VectorType subclasses
This seems to have basically broken both ABI and API of llvm-c. 

Joerg
_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

----- End forwarded message -----

Agreed. Those llvm-c changes are wrong, and compatibility needs to be
maintained for the numeric values at minimum. Probably also would be a good
idea to make LLVMVectorTypeKind a deprecated alias
for LLVMFixedVectorTypeKind.

On Wed, May 13, 2020 at 9:11 AM Joerg Sonnenberger via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Bringing this up on llvm-dev for more general attention.
>
> The problem here is two fold:
>
> (1) Reuse of enumeration values is just a major no-go.
> (2) I'm not sure why the existing vector types had to be killed
> completely.
>
> But something clearly has to be done here. This majorly affects e.g.
> Mesa.
>
> Joerg
>
> ----- Forwarded message from Joerg Sonnenberger via llvm-commits <
> llvm-commits at lists.llvm.org> -----
>
> Date: Sun, 10 May 2020 02:34:12 +0200
> From: Joerg Sonnenberger via llvm-commits <llvm-commits at
lists.llvm.org>
> To: Christopher Tetreault <ctetreau at quicinc.com>, Christopher
Tetreault <
> llvmlistbot at llvm.org>
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses
> Reply-To: Joerg Sonnenberger <joerg at bec.de>
>
> On Wed, Apr 22, 2020 at 08:59:20AM -0700, Christopher Tetreault via
> llvm-commits wrote:
> >
> > Author: Christopher Tetreault
> > Date: 2020-04-22T08:59:01-07:00
> > New Revision: 2dea3f129878e929e5d1f00b91a622eb1ec8be4e
> >
> > URL:
>
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e
> > DIFF:
>
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e.diff
> >
> > LOG: [SVE] Add new VectorType subclasses
>
> This seems to have basically broken both ABI and API of llvm-c.
>
> Joerg
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> ----- End forwarded message -----
> _______________________________________________
> 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/20200513/ef0360d2/attachment.html>

Joerg,

   First of all, here is a link to the original RFC for this that explains the
motivation: http://lists.llvm.org/pipermail/llvm-dev/2020-March/139811.html. In
short, VectorType used to be a pair of (bool, unsigned), where the bool is true
if the vector is scalable. If the bool is false, the unsigned is the exact
number of elements in the vector. If the bool is true, the unsigned is the
minimum number of elements in the vector. Unfortunately, scalable vectors are a
new concept, and there existed a ton of code that assumed that vectors can only
be fixed width. Thus, the bool was largely ignored, and bugs were everywhere.
The choice was to painstakingly educate everyone, and be constantly vigilant
against new bugs, or to introduce a more strongly typed solution. The new scheme
is much less error prone because (when the refactor is complete), VectorType
will not have a getNumElements() method. Users must either call
getElementCount() which returns the pair that they must then explicitly
acknowledge, or they must cast to FixedVectorType if they want to call
getNumElements(). Either way, at that point if they manage to commit a bug, it
will be due to negligence on their part rather than a "you just had to
know" situation.

   As for point 1, this is a work in progress. Previously, you could construct a
fixed width vector by calling LLVMVectorType(), you could ask if a Type was some
sort of vector by checking if it's LLVMTypeKind is LLVMVectorTypeKind, and
you could get the unsigned value in the ElementCount by calling
LLVMGetVectorSize(). You had no way of knowing if a vector was scalable, or
handling it correctly. Now, you can construct a FixedWidthVector by calling
LLVMVectorType(), you can check if a vector is a fixed width or scalable vector
by inspecting the LLVMTypeKind, and you can correctly handle scalable vectors by
correctly interpreting the return value of LLVMGetVectorSize(). The C api is
strictly more correct than it was previously. Really, the only thing that must
be done to make the API complete is to add a way to construct a scalable vector.
In C++, the recommended way of telling if a vector is a scalable vector is to
call isa<ScalableVectorType>(Ty), so the current C api already closely
resembles this.

   There are only so many hours in the day, and my objective is to fix the
issues with the C++ api. I have not spent a tremendous amount of time thinking
about how to architect the C api for scalable vectors. That said, I have this
patch up https://reviews.llvm.org/D78599 that provides a way to specifically get
the bool from a vector, and to construct a scalable vector. If you need to be
able to construct scalable vector objects via the C api, then feel free to bring
this patch over the finish line or submit a patch that you feel is more in line
with typical C api usage patterns.

Thanks,
   Christopher Tetreault

-----Original Message-----
From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Joerg
Sonnenberger via llvm-dev
Sent: Wednesday, May 13, 2020 6:11 AM
To: llvm-dev at lists.llvm.org
Subject: [EXT] [llvm-dev] [llvm-commits at lists.llvm.org: Re: [llvm] 2dea3f1 -
[SVE] Add new VectorType subclasses]

Bringing this up on llvm-dev for more general attention.

The problem here is two fold:

(1) Reuse of enumeration values is just a major no-go.
(2) I'm not sure why the existing vector types had to be killed completely.

But something clearly has to be done here. This majorly affects e.g.
Mesa.

Joerg

----- Forwarded message from Joerg Sonnenberger via llvm-commits
<llvm-commits at lists.llvm.org> -----

Date: Sun, 10 May 2020 02:34:12 +0200
From: Joerg Sonnenberger via llvm-commits <llvm-commits at lists.llvm.org>
To: Christopher Tetreault <ctetreau at quicinc.com>, Christopher Tetreault
<llvmlistbot at llvm.org>
Cc: llvm-commits at lists.llvm.org
Subject: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses
Reply-To: Joerg Sonnenberger <joerg at bec.de>

On Wed, Apr 22, 2020 at 08:59:20AM -0700, Christopher Tetreault via llvm-commits
wrote:
>
> Author: Christopher Tetreault
> Date: 2020-04-22T08:59:01-07:00
> New Revision: 2dea3f129878e929e5d1f00b91a622eb1ec8be4e
>
> URL:
> https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91
> a622eb1ec8be4e
> DIFF:
> https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91
> a622eb1ec8be4e.diff
>
> LOG: [SVE] Add new VectorType subclasses
This seems to have basically broken both ABI and API of llvm-c.

Joerg
_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

----- End forwarded message -----
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Regarding the numerical value of the LLVMTypeKind enum, my understanding is that
LLVM-C does not promise to maintain ABI compatability between versions. If I am
mistaken, I can fix this issue.

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James Y
Knight via llvm-dev
Sent: Wednesday, May 13, 2020 7:33 AM
To: Joerg Sonnenberger <joerg at bec.de>; llvm-dev <llvm-dev at
lists.llvm.org>
Subject: [EXT] Re: [llvm-dev] [llvm-commits at lists.llvm.org: Re: [llvm]
2dea3f1 - [SVE] Add new VectorType subclasses]

Agreed. Those llvm-c changes are wrong, and compatibility needs to be maintained
for the numeric values at minimum. Probably also would be a good idea to make
LLVMVectorTypeKind a deprecated alias for LLVMFixedVectorTypeKind.

On Wed, May 13, 2020 at 9:11 AM Joerg Sonnenberger via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
Bringing this up on llvm-dev for more general attention.

The problem here is two fold:

(1) Reuse of enumeration values is just a major no-go.
(2) I'm not sure why the existing vector types had to be killed
completely.

But something clearly has to be done here. This majorly affects e.g.
Mesa.

Joerg

----- Forwarded message from Joerg Sonnenberger via llvm-commits
<llvm-commits at lists.llvm.org<mailto:llvm-commits at
lists.llvm.org>> -----

Date: Sun, 10 May 2020 02:34:12 +0200
From: Joerg Sonnenberger via llvm-commits <llvm-commits at
lists.llvm.org<mailto:llvm-commits at lists.llvm.org>>
To: Christopher Tetreault <ctetreau at quicinc.com<mailto:ctetreau at
quicinc.com>>, Christopher Tetreault <llvmlistbot at
llvm.org<mailto:llvmlistbot at llvm.org>>
Cc: llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
Subject: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses
Reply-To: Joerg Sonnenberger <joerg at bec.de<mailto:joerg at
bec.de>>

On Wed, Apr 22, 2020 at 08:59:20AM -0700, Christopher Tetreault via llvm-commits
wrote:
>
> Author: Christopher Tetreault
> Date: 2020-04-22T08:59:01-07:00
> New Revision: 2dea3f129878e929e5d1f00b91a622eb1ec8be4e
>
> URL:
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e
> DIFF:
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e.diff
>
> LOG: [SVE] Add new VectorType subclasses
This seems to have basically broken both ABI and API of llvm-c.

Joerg
_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

----- End forwarded message -----
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto: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/20200513/230622ce/attachment.html>

On Wed, May 13, 2020 at 05:54:04PM +0000, Chris Tetreault
wrote:
>    There are only so many hours in the day, and my objective is to fix
> the issues with the C++ api. I have not spent a tremendous amount of
> time thinking about how to architect the C api for scalable vectors.
> That said, I have this patch up https://reviews.llvm.org/D78599 that
> provides a way to specifically get the bool from a vector, and to
> construct a scalable vector. If you need to be able to construct
> scalable vector objects via the C api, then feel free to bring this
> patch over the finish line or submit a patch that you feel is more in
> line with typical C api usage patterns.
ISTM that not supporting scalable vectors properly in the llvm-c API is
no showstopper right now. But the full breakage of the fixed vector API
is a real concern.

Joerg