<div dir="ltr"><div><br></div><div><div>On Wed, May 13, 2020 at 3:14 PM Chris Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>> wrote:<br></div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">





<div lang="EN-US">
<div>
<p class="MsoNormal">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.</p></div></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div><p class="MsoNormal"><u></u></p>
<p class="MsoNormal">As for not exposing the names, I think that would be a mistake.</p></div></div></blockquote><div><br></div><div><div>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'.<br></div><div><br></div><div>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.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div><p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u></p></div></div></blockquote><div><br></div><div>I agree, it's fine that you haven't implemented full scalable vector support yet in the C API.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div><p class="MsoNormal">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.</p></div></div></blockquote><div><br></div><div>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.</div><div><br></div></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div><p class="MsoNormal"><u></u></p>
<p class="MsoNormal"><b>From:</b> James Y Knight <<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</a>> <br>
<b>Sent:</b> Wednesday, May 13, 2020 11:30 AM<br>
<b>To:</b> Chris Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>><br>
<b>Cc:</b> Joerg Sonnenberger <<a href="mailto:joerg@bec.de" target="_blank">joerg@bec.de</a>>; LLVM Dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br>
<b>Subject:</b> [EXT] Re: [llvm-dev] [<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses]<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Wed, May 13, 2020 at 1:57 PM Chris Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal">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. <u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"><b>From:</b> llvm-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">llvm-dev-bounces@lists.llvm.org</a>>
<b>On Behalf Of </b>James Y Knight via llvm-dev<br>
<b>Sent:</b> Wednesday, May 13, 2020 7:33 AM<br>
<b>To:</b> Joerg Sonnenberger <<a href="mailto:joerg@bec.de" target="_blank">joerg@bec.de</a>>; llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br>
<b>Subject:</b> [EXT] Re: [llvm-dev] [<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses]<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<div>
<p class="MsoNormal">On Wed, May 13, 2020 at 9:11 AM Joerg Sonnenberger via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin:5pt 0in 5pt 4.8pt">
<p class="MsoNormal">Bringing this up on llvm-dev for more general attention.<br>
<br>
The problem here is two fold:<br>
<br>
(1) Reuse of enumeration values is just a major no-go.<br>
(2) I'm not sure why the existing vector types had to be killed<br>
completely.<br>
<br>
But something clearly has to be done here. This majorly affects e.g.<br>
Mesa.<br>
<br>
Joerg<br>
<br>
----- Forwarded message from Joerg Sonnenberger via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> -----<br>
<br>
Date: Sun, 10 May 2020 02:34:12 +0200<br>
From: Joerg Sonnenberger via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
To: Christopher Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>>, Christopher Tetreault <<a href="mailto:llvmlistbot@llvm.org" target="_blank">llvmlistbot@llvm.org</a>><br>
Cc: <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
Subject: Re: [llvm] 2dea3f1 - [SVE] Add new VectorType subclasses<br>
Reply-To: Joerg Sonnenberger <<a href="mailto:joerg@bec.de" target="_blank">joerg@bec.de</a>><br>
<br>
On Wed, Apr 22, 2020 at 08:59:20AM -0700, Christopher Tetreault via llvm-commits wrote:<br>
> <br>
> Author: Christopher Tetreault<br>
> Date: 2020-04-22T08:59:01-07:00<br>
> New Revision: 2dea3f129878e929e5d1f00b91a622eb1ec8be4e<br>
> <br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e" target="_blank">
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e.diff" target="_blank">
https://github.com/llvm/llvm-project/commit/2dea3f129878e929e5d1f00b91a622eb1ec8be4e.diff</a><br>
> <br>
> LOG: [SVE] Add new VectorType subclasses<br>
<br>
This seems to have basically broken both ABI and API of llvm-c. <br>
<br>
Joerg<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
----- End forwarded message -----<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><u></u><u></u></p>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>

</blockquote></div></div></div>