<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Just to chime in here, I generally agree with Eli's framing here.</p>
<p>Additionally, I, as an out of tree user, don't really care about
what names we end up with. Pick what makes sense for the project,
we'll adapt. <br>
</p>
<p>Personally, I have no problem with the current state in tree.
I'd prefer a bit of migration support in the public C API, but we
don't guarantee perfect stability, so the fact that downstream
users might need to change doesn't particularly bug me. <br>
</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 5/22/20 3:34 PM, Eli Friedman via
llvm-dev wrote:<br>
</div>
<blockquote type="cite"
cite="mid:BY5PR02MB70925BA0B46A20A685180171CAB40@BY5PR02MB7092.namprd02.prod.outlook.com">
<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;}
code
{mso-style-priority:99;
font-family:"Courier New";}
span.EmailStyle20
{mso-style-type:personal-reply;
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:912739698;
mso-list-type:hybrid;
mso-list-template-ids:953300846 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l0:level1
{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;}
@list l1
{mso-list-id:1292325734;
mso-list-type:hybrid;
mso-list-template-ids:-2032400168 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l1:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l1:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l1:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1: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]-->
<div class="WordSection1">
<p class="MsoNormal">(reply inline)<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" style="margin-left:.5in"><b>From:</b>
llvm-dev <a class="moz-txt-link-rfc2396E" href="mailto:llvm-dev-bounces@lists.llvm.org"><llvm-dev-bounces@lists.llvm.org></a>
<b>On Behalf Of </b>John McCall via llvm-dev<br>
<b>Sent:</b> Friday, May 22, 2020 12:59 PM<br>
<b>To:</b> Chris Tetreault <a class="moz-txt-link-rfc2396E" href="mailto:ctetreau@quicinc.com"><ctetreau@quicinc.com></a><br>
<b>Cc:</b> <a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><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" style="margin-left:.5in"><o:p> </o:p></p>
<div>
<div>
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif">On 22
May 2020, at 3:15, 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-right:0in;margin-bottom:3.75pt">
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif;color:#777777">John,<br>
<br>
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.<br>
<br>
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></span></p>
</blockquote>
</div>
<div>
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif">If you
define
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">getElementCount()
= delete</span></code><span
style="font-family:"Arial",sans-serif"> in
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">VectorType</span></code><span
style="font-family:"Arial",sans-serif">, you
can easily find the places that are doing this and
update them to use
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">VectorBaseType</span></code><span
style="font-family:"Arial",sans-serif">. You
wouldn’t actually check that in, of course; it’s a tool
for doing the audit in a way that’s no more painstaking
than what you’re already doing with </span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">getNumElements()</span></code><span
style="font-family:"Arial",sans-serif">. And
in the meantime, the code that you haven’t audited — the
code that’s currently unconditionally calling </span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">getNumElements()</span></code><span
style="font-family:"Arial",sans-serif"> on a
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">VectorType</span></code><span
style="font-family:"Arial",sans-serif"> — will
just conservatively not trigger on scalable vectors,
which for most of LLVM is a better result than crashing
if a scalable vector comes through until your audit gets
around to updating it.<o:p></o:p></span></p>
<p>I think there are two separate aspects here, that we
shouldn’t mix together:<o:p></o:p></p>
<ol type="1" start="1">
<li style="mso-list:l1 level1 lfo1">How do we get to a
consistent state in-tree, in llvm-project, where code
that requires fixed-length vectors only handles
fixed-length vectors?<o:p></o:p></li>
<li style="mso-list:l1 level1 lfo1">What names do we
expose for out-of-tree users?<o:p></o:p></li>
</ol>
<p>If we want to simply rename the types that currently
exist in-tree
(VectorType/FixedVectorType/ScalableVectorType), we can do
that mechanically in a few patches; we can use a few “sed”
invocations, then clang-format the result. This would
allow us to preserve the old meaning of the name
“VectorType” for out-of-tree code. I don’t think this is
particularly valuable; the names on trunk seem fine, and
out-of-tree code can equally use “sed” in the opposite
direction.<o:p></o:p></p>
<p>In terms of semantic changes, I see three alternatives:<o:p></o:p></p>
<ol type="1" start="1">
<li style="mso-list:l0 level1 lfo2">We continue as we are:
VectorType is the base class, and we plan to change any
code which expects a fixed-width vector to use
FixedVectorType instead.<o:p></o:p></li>
<li style="mso-list:l0 level1 lfo2">We “typedef VectorType
BaseVectorType;”, go through and change all the places
where we expect a BaseVectorType, then change the
meaning of VectorType back to its original meaning of a
fixed-width vector. I think this is problematic,
though. As this work is in progress, it would be hard
to keep track of whether code in the tree using the name
VectorType means to use a FixedVectorType, or a
BaseVectorType. And the patch that actually changes the
meaning of VectorType would be a big functional change
(even if it’s not actually a big patch).<o:p></o:p></li>
<li style="mso-list:l0 level1 lfo2">We completely kill off
uses of the name “VectorType” in-tree: incrementally
rename every use of the name to either BaseVectorType or
FixedVectorType.<o:p></o:p></li>
</ol>
</div>
<p>The last alternative is sort of more formal: it involves
going through and explicitly making a choice everywhere.
But I don’t think continuing as we are is a problem.<o:p></o:p></p>
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif;color:#777777">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></span></p>
</div>
<div>
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif">I’m
responding to this as soon as I heard about it. I’ll
accept that ideally I would have seen it when you raised
the RFC in March, although in practice it’s quite hard to
proactively keep up with llvmdev, and as a community I
think we really need to figure out a better process for IR
design. I’m not going to feel guilty about work you did
for over a month without raising an RFC. And I really
don’t think you have in any way wasted your time; I am
asking for a large but fairly mechanical change to the
code you’ve already been updating.<o:p></o:p></span></p>
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif">But most
of your arguments are not based on how much work you’ve
done on your current audit, they’re based on the fact that
scalable vectors were initially implemented as a flag on
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">VectorType</span></code><span
style="font-family:"Arial",sans-serif">. So part
of my problem here is that you’re basically arguing that,
as soon as that was accepted, the generalization of </span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">VectorType</span></code><span
style="font-family:"Arial",sans-serif"> was
irreversible; and that’s a real problem, because it’s very
common for early prototype work to not worry much about
representations, and so they stumble into this kind of
problematic representation.<o:p></o:p></span></p>
<p style="margin-left:.5in"><span
style="font-family:"Arial",sans-serif">My
concern is really only ~50% that this is going to force a
lot of unnecessary mechanical changes for downstream
projects and 50% that generalizing
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">VectorType</span></code><span
style="font-family:"Arial",sans-serif"> to
include scalable vectors, as the initial prototype did, is
the wrong polarity and makes a lot of existing code broken
if it ever sees a scalable vector. Your hierarchy change
only solves this in the specific case that there’s an
immediate call to
</span><code><span
style="font-size:10.0pt;color:black;background:#F7F7F7">getNumElements()</span></code><span
style="font-family:"Arial",sans-serif">.<o:p></o:p></span></p>
<p>I had similar concerns about the “polarity” initially. But
we’ve found that, in practice, that making the default
“wrong” in IR optimizations has been helpful for making
progress on various aspects in parallel. So we can take C
code using intrinsics, and produce assembly, even though we
haven’t fixed all the issues in the bits in between.<o:p></o:p></p>
<p>Practically speaking, almost all places that specifically
need a fixed-length type either call getNumElements(), or
make some sort of query about the size of the type. So I
don’t think there’s a big invisible tail of work even if we
have some code that’s temporarily wrong.<o:p></o:p></p>
<p>-Eli<o:p></o:p></p>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
</body>
</html>