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

Wed May 13 15:59:42 PDT 2020

On Wed, May 13, 2020 at 3:14 PM Chris Tetreault <ctetreau at quicinc.com>
wrote:

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

We attempt not to break ABI and API, if we don't need to. It is not an
ironclad guarantee, because sometimes things may change so fundamentally on
the C++ side, so as to make keeping C API compatibility effectively
impossible. But it is "best effort" attempt. I'm sorry you got contrary
advice from a reviewer.

As for not exposing the names, I think that would be a mistake.
>

I'm not sure what you mean by this. I want you to expose the new scalable
vector kind as LLVMScalableVectorTypeKind (as you have), and put it at the
end of the list to not change existing enum values. I want you to leave the
existing C API name LLVMVectorTypeKind as referring to the renamed on the
C++ side 'Type::FixedVectorTyID'.

If, later on, it seems useful to also use the new "FixedVectorType"
terminology in the C API, that can be done. When doing so, be consistent,
and also rename the constructor from "LLVMVectorType" to
"LLVMFixedVectorType". Additionally, please leave the old names as
deprecated aliases. It doesn't feel to me that such a renaming is really
worthwhile, though, and I'd suggest it best to just leave the old names
alone.

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.
>
>
I agree, it's fine that you haven't implemented full scalable vector
support yet in the C API.

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

I don't have a codebase affected by this, I care because users out in the
wide world do have such codebases, and we have a policy of not breaking
them unless it's necessary. If you'd like to do more work and fully
implement the proper LLVM-C interfaces to support scalable vectors, that's
fine too. But I'm not asking for that, because it would be
new functionality, rather than simply not breaking old functionality.

*From:* James Y Knight <jyknight at google.com>
> *Sent:* Wednesday, May 13, 2020 11:30 AM
> *To:* Chris Tetreault <ctetreau at quicinc.com>
> *Cc:* 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]
>
>
>
> 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>
> 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> *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> 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/bb77ae20/attachment.html>