[llvm-dev] [llvm-commits@lists.llvm.org: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses]
Joerg Sonnenberger via llvm-dev
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-c...@lists.llvm.org> -----
Date: Sun, 10 May 2020 02:34:12 +0200
From: Joerg Sonnenberger via llvm-commits <llvm-c...@lists.llvm.org>
To: Christopher Tetreault <ctet...@quicinc.com>, Christopher Tetreault <llvml...@llvm.org>
Cc: llvm-c...@lists.llvm.org
Subject: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses
Reply-To: Joerg Sonnenberger <jo...@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-c...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
----- End forwarded message -----
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
James Y Knight via llvm-dev
Chris Tetreault via llvm-dev
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
Chris Tetreault via 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.
James Y Knight via llvm-dev
Renato Golin via llvm-dev
<llvm...@lists.llvm.org> 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.
The way I see it, the enum change brought no positive outcome, so
shouldn't be done. It's not because we don't guarantee backward
compatibility that we should trivially break it for no benefit.
Joerg Sonnenberger via llvm-dev
> 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
Chris Tetreault via llvm-dev
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.
Chris Tetreault via llvm-dev
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
James Y Knight via llvm-dev
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.