<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 21, 2020 at 2:22 PM Chris Tetreault via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">





<div lang="EN-US">
<div class="gmail-m_-5823707428565208209WordSection1">
<p class="MsoNormal">John,<u></u><u></u></p>
<p>> 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.<u></u><u></u></p>
<p class="MsoNormal">While I’m at a loss to find a documented policy, I recall this thread (<a href="http://lists.llvm.org/pipermail/llvm-dev/2020-February/139207.html" target="_blank"><span style="color:windowtext;text-decoration:none">http://lists.llvm.org/pipermail/llvm-dev/2020-February/139207.html</span></a>)
 where this claim was made and not disputed. </p></div></div></blockquote><div><br></div><div><div style="color:rgb(0,0,0)">John description is maybe slightly more conservative than how I've been seeing it: there is always a tension there and some balance to find, but we usually err on the side of doing what's best upstream, while avoiding "unnecessary churn". Even without over-emphasizing downstream, other folks upstream are working on patches and too much churn on the main APIs makes everyone else's on-going patches harder to carry over.</div><div style="color:rgb(0,0,0)"><br></div><div style="color:rgb(0,0,0)">In the case of this feature, the angle seems maybe more that this is introducing something experimental and we don't how much extra churn will occur until this stabilize. With this angle the priority is to reduce the disruption on the "stable" long-lived VectorType: it isn't immutable but you don't want to change it every other months for the new couple of years.</div><div style="color:rgb(0,0,0)"><br></div><div style="color:rgb(0,0,0)">Ultimately I'd rather still consider what makes the most sense upstream for the design (where do we want to be in two years) and try to get there. This does not require to preserve the APIs, but the chose path to get should likely favor doing "some" extra churn/detour in introducing the new concepts when it reduces significantly the churn to the "stable" existing classes/API. That may fit what John considered under "we do make some effort to keep existing source interfaces"? This is how I understand the approach for "pointer-authentication feature" that was mentioned as an example.</div></div><div style="color:rgb(0,0,0)"><br></div><div style="color:rgb(0,0,0)">Hopefully this makes sense?</div><div style="color:rgb(0,0,0)"><br></div><div style="color:rgb(0,0,0)">Thanks again for all the refactoring work to introduce SVE Chris! This is no small task :)</div><div style="color:rgb(0,0,0)"><br></div><div style="color:rgb(0,0,0)">Best,</div><div style="color:rgb(0,0,0)"><br></div><div style="color:rgb(0,0,0)">-- </div><div style="color:rgb(0,0,0)">Mehdi</div><div style="color:rgb(0,0,0)"><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_-5823707428565208209WordSection1"><p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">> … Probably 99% of the code using VectorType semantically requires it to be a fixed-width vector.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">> The generalization of VectorType to scalable vector types was a representational shortcut that should never have been allowed<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">I agree. Unfortunately it happened, and our choices are to fix it or accept the technical debt forever.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">> 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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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.
<u></u><u></u></p>
<p>> Are you actually auditing and testing them all to work for scalable vector types, or are you just fixing the obvious compile failures?
<u></u><u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p>> “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.<u></u><u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Thanks,<u></u><u></u></p>
<p class="MsoNormal">   Christopher Tetreault<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div style="border-style:solid none none;border-top-width:1pt;border-top-color:rgb(225,225,225);padding:3pt 0in 0in">
<p class="MsoNormal"><b>From:</b> John McCall <<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>> <br>
<b>Sent:</b> Thursday, May 21, 2020 1:47 PM<br>
<b>To:</b> Chris Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>><br>
<b>Cc:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> [EXT] Re: [llvm-dev] [RFC] Refactor class hierarchy of VectorType in the IR<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p><span style="font-family:Arial,sans-serif">On 21 May 2020, at 16:01, Chris Tetreault wrote:<u></u><u></u></span></p>
</div>
<div>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(119,119,119);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">Hi John,<br>
<br>
I’d like to address some points in your message.<u></u><u></u></span></p>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(153,153,153);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(153,153,153)">Practically speaking, this is going to break every out-of-tree frontend, backend, or optimization pass that supports SIMD types.<u></u><u></u></span></p>
</blockquote>
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">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.<u></u><u></u></span></p>
</blockquote>
</div>
<div>
<p><span style="font-family:Arial,sans-serif">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.<u></u><u></u></span></p>
</div>
<div>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(119,119,119);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">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.<u></u><u></u></span></p>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(153,153,153);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(153,153,153)">… by having the VectorType type semantically repurposed out from under them.<u></u><u></u></span></p>
</blockquote>
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">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.<u></u><u></u></span></p>
</blockquote>
</div>
<div>
<p><span style="font-family:Arial,sans-serif">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
</span><code><span style="font-size:10pt;color:black;background-color:rgb(247,247,247)">getVectorElementType()</span></code><span style="font-family:Arial,sans-serif"> accessor that checks for both and otherwise returns null.<u></u><u></u></span></p>
</div>
<div>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(119,119,119);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(153,153,153);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(153,153,153)">… a particular largely-vendor-specific extension …<u></u><u></u></span></p>
</blockquote>
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">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.<u></u><u></u></span></p>
</blockquote>
</div>
<div>
<p><span style="font-family:Arial,sans-serif">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.<u></u><u></u></span></p>
</div>
<div>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(119,119,119);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(153,153,153);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(153,153,153)">… it’s much better for code that does support both to explicitly opt in by checking for and handling the more general type …<u></u><u></u></span></p>
</blockquote>
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">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.<u></u><u></u></span></p>
</blockquote>
</div>
<div>
<p><span style="font-family:Arial,sans-serif">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.<u></u><u></u></span></p>
</div>
<div>
<blockquote style="border-style:none none none solid;border-left-width:1.5pt;border-left-color:rgb(119,119,119);padding:0in 0in 0in 4pt;margin-left:0in;margin-right:0in;margin-bottom:3.75pt">
<p><span style="font-family:Arial,sans-serif;color:rgb(119,119,119)">I think that it is important to ensure that things have clear sensible names, and to clean up historical baggage when the opportunity presents.<u></u><u></u></span></p>
</blockquote>
</div>
<div>
<p><span style="font-family:Arial,sans-serif">“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
</span><code><span style="font-size:10pt;color:black;background-color:rgb(247,247,247)">clang::PointerType</span></code><span style="font-family:Arial,sans-serif"> 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.<u></u><u></u></span></p>
<p><span style="font-family:Arial,sans-serif">You have resigned yourself to doing a lot of work in pursuit of something that I really don’t think is actually an improvement.<u></u><u></u></span></p>
<p><span style="font-family:Arial,sans-serif">John.<u></u><u></u></span></p>
</div>
</div>
</div>
</div>

_______________________________________________<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" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div></div></div></div>