<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=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 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";
color:black;}
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;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman","serif";
color:black;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.EmailStyle18
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.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;}
--></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 bgcolor="white" lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Hi Philip,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">I’m not sure I understand your concerns exactly.
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">My proposal is to construct AvailableFunctionAttrs, for example, with an initial size that comprehends the upper limit of the number of enum attribute slots in Attribute:AttrKind …<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D">class AttributeListImpl final<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> : public FoldingSetNode,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> private TrailingObjects<AttributeListImpl, AttributeSet> {<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> friend class AttributeList;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> friend TrailingObjects;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D">private:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> …<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> /// Bitset with a bit for each available attribute Attribute::AttrKind.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D">
<span style="background:yellow;mso-highlight:yellow">std::bitset<Attribute::EndAttrKinds></span> AvailableFunctionAttrs;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D"> …<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Courier New";color:#1F497D">};<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="color:#1F497D"><o:p> </o:p></span></a></p>
<p class="MsoNormal"><span style="color:#1F497D">Yes, I suspect that the footprint of a bitset implementation is bigger than 1 or 2 uint64_t words, but I’m wary of dictating an arbitrary limit on the number of enum attributes allowed based on how we implement
the API for keeping track of whether a function or data object has an attribute or not.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">If I’m misunderstanding what you mean by “pay[ing] an extra word to integrate them cleanly,” then can you please clarify?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">~ Todd<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext"> Philip Reames [mailto:listmail@philipreames.com]
<br>
<b>Sent:</b> Friday, April 5, 2019 11:45 AM<br>
<b>To:</b> Snider, Todd; llvm-dev<br>
<b>Subject:</b> [EXTERNAL] Re: [llvm-dev] [RFC] Proposed update to convert two 64-bit attribute bitmasks to std::bitset<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p>The representation change seems fine per se, but we still have a concern with the 65th attribute increasing the size of the objects in question. Do you have a plan to minimize that impact?<o:p></o:p></p>
<p>To be clear, I think the representation change is fine regardless. As one example, we have a bunch of downstream attributes, and would happily pay the extra word to integrate them cleanly with the built in variety in our downstream code. I'm sure we're
not the only ones.<o:p></o:p></p>
<p>Philip<o:p></o:p></p>
<div>
<p class="MsoNormal">On 4/4/19 3:38 PM, Snider, Todd via llvm-dev wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Arial","sans-serif";background:white"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="background:white">There are two 64-bit bitmasks maintained in </span><a href="https://sdocc.itg.ti.com/ui#file:review=11893/version=393846" title="Open AttributeImpl.h"><span style="background:white">AttributeImpl.h</span></a><span style="background:white">:</span><br>
<br>
<span style="background:white">- AvailableFunctionAttrs is part of the AttributeListImpl class, and</span><br>
<span style="background:white">- AvailableAttrs is part of the AttributeSetNode class</span><br>
<br>
<span style="background:white">Both of these assume that the number of available enum attributes is limited to 64. In fact, a static_assert in </span><a href="https://sdocc.itg.ti.com/ui#file:review=11893/version=393848" title="Open Attributes.cpp"><span style="background:white">Attributes.cpp</span></a><span style="background:white"> enforces
that the number of enum attributes stays at 64 or below. However, the bitcode writer and reader don't communicate enum attributes via bitmask anymore. Enum attributes are encoded in attribute groups.</span><br>
<br>
<span style="background:white">The AvailableFunctionAttrs and AvailableAttrs bitmasks are leftovers that need to be updated to remove the limitation on the number of enum attributes that can be defined in the Attribute::AttrKind enum.</span><br>
<br>
<span style="background:white">Per a suggestion that I received a while ago from Reid Kleckner (on the llvm-dev list), I propose to implement both of these data members as std::bitset objects.</span><br>
<br>
<span style="background:white">Here are the details for this proposed change:</span><br>
<br>
<span style="background:white">llvm/lib/IR/</span><a href="https://sdocc.itg.ti.com/ui#file:review=11893/version=393846" title="Open AttributeImpl.h"><span style="background:white">AttributeImpl.h</span></a><span style="background:white">:</span><br>
<span style="background:white">- Define AvailableAttrs and AvailableFunctionAttrs with type std::bitset<AttributeEndAttrKinds> instead of uint64_t</span><br>
<span style="background:white">- Update AttributeSetNode::hasAttribute() to use the std::bitset test function on AvailableAttrs to check if an enum attribute is present</span><br>
<span style="background:white">- Update AttributeListImpl::hasFnAttribute() to use the std::bitset test function on AvailableFunctionAttrs to check if an enum attribute is present</span><br>
<br>
<span style="background:white">llvm/lib/IR/</span><a href="https://sdocc.itg.ti.com/ui#file:review=11893/version=393848" title="Open Attributes.cpp"><span style="background:white">Attributes.cpp</span></a><span style="background:white">:</span><br>
<span style="background:white">- Update AttributeSetNode::AttributeSetNode() constructor to use the std::bitset's set function to initialize AvailableAttrs</span><br>
<span style="background:white">- Update AttributeListImpl::AttributeListImpl() constructor to use the std::bitset's set function to initialize AvailableFunctionAttrs and remove the static_assert that enforces an upper limit of 64 on the number of enum attributes
allowed</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman","serif""><br>
<br>
<o:p></o:p></span></p>
<pre>_______________________________________________<o:p></o:p></pre>
<pre>LLVM Developers mailing list<o:p></o:p></pre>
<pre><a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><o:p></o:p></pre>
<pre><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></pre>
</blockquote>
</div>
</body>
</html>