<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:"Courier New";}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle19
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle22
{mso-style-type:personal-compose;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:1438719672;
mso-list-type:hybrid;
mso-list-template-ids:1303423638 67698705 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l0:level1
{mso-level-text:"%1\)";
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">John,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"> For the last several months, those of us working on the scalable vectors feature have been examining the codebase, identifying places where llvm::VectorType is used incorrectly, and fixing them. The fact is that there are many places
where VectorType is correctly taken to be the generic “any vector” type. getNumElements may be being called, but it’s being called in accordance with the previously documented semantics. There are many places where it isn’t as well, and many people add new
usages that are incorrect.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"> This puts us in an unfortunate situation: if we were to take your proposal and have VectorType be the fixed width vector type, then all of this work is undone. Every place that has been fixed up to correctly have VectorType be used as
a universal vector type will now incorrectly have the fixed width vector type being used as the universal vector type. Since VectorType will inherit from BaseVectorType, it will have inherited the getElementCount(), so the compiler will happily continue to
compile this code. However, none of this code will even work with scalable vectors because the bool will always be false. There will be no compile time indication that this is going on, functions will just start mysteriously returning nullptr. Earlier this
afternoon, I set about seeing how much work it would be to change the type names as you have suggested. I do not see any way forward other than painstakingly auditing the code.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"> On the other hand, creating a new fixed width vector type has a clear upgrade path. 1) delete getNumElements() from the base class locally. 2) try to build 3) fix the failures, uploading patches for these fixes 4) once step 3 is completed
throughout the codebase, merge the patch to remove getNumElements() from VectorType. Downstream and out-of-tree codebases have this upgrade path as well. There exists no easy upgrade path if we go the other way and have VectorType become a specifically fixed-width
vector type.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"> Basically, I believe that the best thing to do is to move forward with the type names that I have proposed:<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1">The type names more accurately represent the usage of the types.<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1">Changing course now would result in a tremendous amount of rework being done in the upstream codebase. This will have a significant impact on the pace of development in upstream.<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1">The process for completing the changes would be much easier if we use the types I propose. The compiler can tell you if you’re using getNumElements() on a potentially scalable vector.
The compiler cannot tell you that SomeFixedVector->getElementCount().Scalable and isa<ScalableVectorType>(SomeFixedVector) are always false.<o:p></o:p></li></ol>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"> Additionally, for those who disagree that the LLVM developer policy is to disregard the needs of downstream codebases when making changes to upstream, I submit that not throwing away months of work by everybody working to fix the codebase
to handle scalable vectors represents a real expected benefit. I personally have been spending most of my time on this since January.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
<p class="MsoNormal"> Christopher Tetreault<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> John McCall <rjmccall@apple.com> <br>
<b>Sent:</b> Thursday, May 21, 2020 3:32 PM<br>
<b>To:</b> Chris Tetreault <ctetreau@quicinc.com><br>
<b>Cc:</b> llvm-dev@lists.llvm.org<br>
<b>Subject:</b> [EXT] Re: [llvm-dev] [RFC] Refactor class hierarchy of VectorType in the IR<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p><span style="font-family:"Arial",sans-serif">On 21 May 2020, at 17:22, Chris Tetreault wrote:<o:p></o:p></span></p>
</div>
<div>
<blockquote style="border:none;border-left:solid #777777 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#777777">John,<o:p></o:p></span></p>
<blockquote style="border:none;border-left:solid #999999 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#999999">This is not categorically true, no. When we make changes that require large-scale updates for downstream codebases, we do so because there’s a real expected benefit to it. For the most part, we do
make some effort to keep existing source interfaces stable.<o:p></o:p></span></p>
</blockquote>
<p><span style="font-family:"Arial",sans-serif;color:#777777">While I’m at a loss to find a documented policy, I recall this thread (</span><a href="http://lists.llvm.org/pipermail/llvm-dev/2020-February/139207.html"><span style="font-family:"Arial",sans-serif;color:#777777">http://lists.llvm.org/pipermail/llvm-dev/2020-February/139207.html</span></a><span style="font-family:"Arial",sans-serif;color:#777777">)
where this claim was made and not disputed. The expected real benefits to this change are: 1) The names now match the semantics 2) It is now statically impossible to accidentally get the fixed number of elements from a scalable vector and 3) It forces everybody
to fix their broken code. If we provided stability guarantees to downstream and out-of-tree codebases, then I might not agree that 3 is a benefit, but my understanding is that we do not provide this guarantee.<o:p></o:p></span></p>
<blockquote style="border:none;border-left:solid #999999 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#999999">… Probably 99% of the code using VectorType semantically requires it to be a fixed-width vector.<o:p></o:p></span></p>
</blockquote>
<p><span style="font-family:"Arial",sans-serif;color:#777777">This code is all broken already. I don’t think supporting common misuse of APIs in a codebase that does not provide stability guarantees is something we should be doing.<o:p></o:p></span></p>
<blockquote style="border:none;border-left:solid #999999 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#999999">The generalization of VectorType to scalable vector types was a representational shortcut that should never have been allowed<o:p></o:p></span></p>
</blockquote>
<p><span style="font-family:"Arial",sans-serif;color:#777777">I agree. Unfortunately it happened, and our choices are to fix it or accept the technical debt forever.<o:p></o:p></span></p>
</blockquote>
</div>
<div>
<p><span style="font-family:"Arial",sans-serif">Perhaps this is part of the root of our disagreement. I consider scalable vector types to be an experimental/unstable feature, as many features are when they’re first added to the compiler. I have much lower standards
for disrupting the early adopters of features like that; if scalable vectors need to be split out of
</span><code><span style="font-size:10.0pt;color:black;background:#F7F7F7">VectorType</span></code><span style="font-family:"Arial",sans-serif">, we should just do it.<o:p></o:p></span></p>
<p><span style="font-family:"Arial",sans-serif">Analogously, when we upstream the pointer-authentication feature, we will be upstreaming a rather flawed representation that I definitely expect us to revise after the fact. That will be problematic for early
adopters of LLVM’s pointer authentication support, but that’s totally acceptable.<o:p></o:p></span></p>
<p><span style="font-family:"Arial",sans-serif">John.<o:p></o:p></span></p>
</div>
<div>
<blockquote style="border:none;border-left:solid #777777 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<blockquote style="border:none;border-left:solid #999999 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#999999">the burden of generalization should usually fall on the people who need to use the generalization, and otherwise we should aim to keep APIs stable when there’s no harm to it.<o:p></o:p></span></p>
</blockquote>
<p><span style="font-family:"Arial",sans-serif;color:#777777">The burden of creating the generalization should be placed on those who need it, I agree. However, once the generalization is in place, the burden is on everybody to use it correctly. The reason
I’ve undertaken this refactor is because I found myself playing whack-a-mole with people adding new broken code after the fact. The previous API was very easy to misuse, so I don’t blame those people.<o:p></o:p></span></p>
<blockquote style="border:none;border-left:solid #999999 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#999999">Are you actually auditing and testing them all to work for scalable vector types, or are you just fixing the obvious compile failures?<o:p></o:p></span></p>
</blockquote>
<p><span style="font-family:"Arial",sans-serif;color:#777777">Everywhere that VectorType::getNumElements() is being called, we are either changing the code to cast to FixedVectorType, or we are updating the code to handle scalable vectors correctly. If the
call site does not have test coverage with scalable vectors, this test coverage is being added. Even for obviously correct transformations such as `VectorType::get(SomeTy, SomeVecTy->getNumElements())` -> `VectorType::get(SomeTy, SomeVecTy->getElementCount())`,
I have been required in code review to provide test coverage. We are taking this seriously.<o:p></o:p></span></p>
<blockquote style="border:none;border-left:solid #999999 1.5pt;padding:0in 0in 0in 4.0pt;margin-left:0in;margin-top:5.0pt;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:"Arial",sans-serif;color:#999999">“Vector” has a traditional and dominant meaning as a fixed-width SIMD type, and the fact that you’ve introduced a generalization doesn’t change that. Clang supports multiple kinds of pointer, but
we still reserve clang::PointerType for C pointers instead of making it an abstract superclass, thus letting our sense of logic introduce a million bugs through accidental generalization throughout the compiler.<o:p></o:p></span></p>
</blockquote>
<p><span style="font-family:"Arial",sans-serif;color:#777777">Various languages implemented on top of LLVM have various different pointer types. However, the LLVM IR language only has one pointer type. The LLVM IR language has two types of vectors, and I think
it’s reasonable to model them as a class hierarchy in this manner. I’m not familiar with the clang::PointerType situation, so I cannot pass judgement on it.<br>
<br>
Thanks,<br>
Christopher Tetreault<br>
<br>
From: John McCall <</span><a href="mailto:rjmccall@apple.com"><span style="font-family:"Arial",sans-serif">rjmccall@apple.com</span></a><span style="font-family:"Arial",sans-serif;color:#777777">><br>
Sent: Thursday, May 21, 2020 1:47 PM<br>
To: Chris Tetreault <</span><a href="mailto:ctetreau@quicinc.com"><span style="font-family:"Arial",sans-serif">ctetreau@quicinc.com</span></a><span style="font-family:"Arial",sans-serif;color:#777777">><br>
Cc: </span><a href="mailto:llvm-dev@lists.llvm.org"><span style="font-family:"Arial",sans-serif">llvm-dev@lists.llvm.org</span></a><span style="font-family:"Arial",sans-serif;color:#777777"><br>
Subject: [EXT] Re: [llvm-dev] [RFC] Refactor class hierarchy of VectorType in the IR<br>
<br>
<br>
On 21 May 2020, at 16:01, Chris Tetreault wrote:<br>
<br>
Hi John,<br>
<br>
I’d like to address some points in your message.<br>
<br>
Practically speaking, this is going to break every out-of-tree frontend, backend, or optimization pass that supports SIMD types.<br>
<br>
My understanding is that the policy in LLVM development is that we do not let considerations for downstream and out-of-tree codebases affect the pace of development.<br>
<br>
This is not categorically true, no. When we make changes that require large-scale updates for downstream codebases, we do so because there’s a real expected benefit to it. For the most part, we do make some effort to keep existing source interfaces stable.<br>
<br>
The C++ API is explicitly unstable. I maintain a downstream fork of LLVM myself, so I know the pain that this is causing because I get to fix all the issues in my internal codebase. However, the fix for downstream codebases is very simple: Just find all the
places where it says VectorType, and change it to say FixedVectorType.<br>
<br>
… by having the VectorType type semantically repurposed out from under them.<br>
<br>
The documented semantics of VectorType prior to my RFC were that it is a generalization of all vector types. The VectorType contains an ElementCount, which is a pair of (bool, unsigned). If the bool is true, then the return value of getNumElements() is the
minimum number of vector elements. If the bool is false, then it is the actual number of elements. My RFC has not changed these semantics. It will eventually delete a function that has been pervasively misused throughout the codebase, but the semantics remain
the same. You are proposing a semantic change to VectorType to have it specifically be a fixed width vector.<br>
<br>
Probably 99% of the code using VectorType semantically requires it to be a fixed-width vector. The generalization of VectorType to scalable vector types was a representational shortcut that should never have been allowed; it should always have used a different
type. Honestly, I’m not convinced your abstract base type is even going to be very useful in practice vs. just having a getVectorElementType() accessor that checks for both and otherwise returns null.<br>
<br>
… a particular largely-vendor-specific extension …<br>
<br>
All SIMD vectors are vendor specific extensions. Just because most of the most popular architectures have them does not make this not true. AArch64 and RISC-V have scalable vectors, so it is not just one architecture. It is the responsibility of all developers
to ensure that they use the types correctly. It would be nice if the obvious thing to do is the correct thing to do.<br>
<br>
I’m not saying that we shouldn’t support scalable vector types. I’m saying that the burden of generalization should usually fall on the people who need to use the generalization, and otherwise we should aim to keep APIs stable when there’s no harm to it.<br>
<br>
… it’s much better for code that does support both to explicitly opt in by checking for and handling the more general type …<br>
<br>
This is how it will work. I am in the process of fixing up call sites that make fixed width assumptions so that they use FixedVectorType.<br>
<br>
Are you actually auditing and testing them all to work for scalable vector types, or are you just fixing the obvious compile failures? Because scalable vector types impose some major restrictions that aren’t imposed on normal vectors, and the static type system
isn’t going to catch most of them.<br>
<br>
I think that it is important to ensure that things have clear sensible names, and to clean up historical baggage when the opportunity presents.<br>
<br>
“Vector” has a traditional and dominant meaning as a fixed-width SIMD type, and the fact that you’ve introduced a generalization doesn’t change that. Clang supports multiple kinds of pointer, but we still reserve clang::PointerType for C pointers instead of
making it an abstract superclass, thus letting our sense of logic introduce a million bugs through accidental generalization throughout the compiler.<br>
<br>
You have resigned yourself to doing a lot of work in pursuit of something that I really don’t think is actually an improvement.<br>
<br>
John.<o:p></o:p></span></p>
</blockquote>
</div>
</div>
</div>
</body>
</html>