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

Wed May 13 10:54:04 PDT 2020

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