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

Wed May 13 14:20:12 PDT 2020

It has been pointed out to me that my choice of words could easily be misinterpreted to mean that the people I am responding to do not care about the quality of the code. I’d like to apologize for this.

When I say “It seems to me that you don’t actually care about constructing scalable vectors …”, I mean that to complete your work, you do not need to be able to construct scalable vectors. I do not want to imply that you do not care about the quality of the code, or that this feature works.

Thanks,
   Christopher Tetreault
From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Chris Tetreault via llvm-dev
Sent: Wednesday, May 13, 2020 12:15 PM
To: James Y Knight <jyknight at google.com>
Cc: 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]

The benefit of not having the gap is not having to spend a bunch of time and effort on ABI maintenance. I actually left LLVMVectorTypeKind in at first, and was asked to remove it in code review. Then, I left a gap in the enum where LLVMVectorTypeKind used to be and was asked in code review to remove the gap.

As for not exposing the names, I think that would be a mistake. The whole point of refactoring vector types is to expose the fact that scalable vectors exist and have to be dealt with.  If a ScalableVectorType object is handed to you from outside of your code, you need to be able to deal with it. Having the ability to correctly handle a scalable vector and having the ability to create one are two separate things. The C api currently has the first ability but not the second. Adding the ability to construct scalable vectors from C still remains to be implemented. There’s a lot of outstanding work. It all needs to get done, but with limited resources, some tasks will get prioritized over others. I would argue that since support for scalable vectors is still a work in progress, you should probably not be constructing them and passing them into it.

It seems to me that you don’t actually care about constructing scalable vectors (otherwise you’d be asking me to complete the work rather than back it out), so the C api should fully support your use case. You should just update your codebase to correctly handle scalable vs fixed width vectors. That way, when the day comes that LLVM fully supports scalable vectors, you will be ready.

From: James Y Knight <jyknight at google.com<mailto:jyknight at google.com>>
Sent: Wednesday, May 13, 2020 11:30 AM
To: Chris Tetreault <ctetreau at quicinc.com<mailto:ctetreau at quicinc.com>>
Cc: Joerg Sonnenberger <joerg at bec.de<mailto:joerg at bec.de>>; LLVM Dev <llvm-dev at lists.llvm.org<mailto: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]

We can't guarantee everything. But, the goal is to not break things in the C API, unless necessary. And, when it is necessary, to make sure it's as obvious as possible of an ABI breakage. E.g., we might delete a function, and create a new one with a new name, instead of incompatibly changing the number of arguments for an existing function name. Or, might leave a gap in enum values where an item is removed.

For your change, modifying the numeric values of the existing constants is an unnecessary change, and a not-obvious ABI-incompatibility. Just add LLVMScalableVectorTypeKind to the end, instead, and it's fine.

And, since the C API basically doesn't support scalable vectors yet, and since you aren't modifying the rest of the C API's uses of "VectorType" to FixedVectorType (e.g. the LLVMVectorType function is still named that), I'd suggest simply leaving the enum value for Fixed vectors named "LLVMVectorTypeKind" as it was before, instead of renaming it to LLVMFixedVectorTypeKind.

On Wed, May 13, 2020 at 1:57 PM Chris Tetreault <ctetreau at quicinc.com<mailto:ctetreau at quicinc.com>> wrote:
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<mailto: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<mailto:joerg at bec.de>>; llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>>
Subject: [EXT] Re: [llvm-dev] [llvm-commits at lists.llvm.org<mailto: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/d879c586/attachment.html>