<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Getting back to this after a two week hiatus...</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Sat, Mar 25, 2017 at 3:17 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Reid,<br>
<br>
Thank for you taking a close look at this area.  Attributes definitely need a good amount of love.  As you point out, the naming of some of the code involved is more than a bit confusing and overly abstracted.  However, there's also a delicate balance here in terms of having a generic understandable API.  In general, I really like your proposal, a few comments inline.</blockquote><div><br></div><div>Thanks!</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><div class="gmail-h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The natural second thing to do is to rename AttributeSetNode to AttributeSet, or<br>
create a pointer wrapper type called AttributeSet and rename today's<br>
AttributeSetNode to AttributeSetImpl. It is inherently an ordered set of<br>
attributes. I also propose to make this type public, as described earlier.<br>
</blockquote></div></div>
SGTM.  From an API design perspective, I'd absolutely love to see AttributeSet die.  The fact that it represents both the set of attributes on a particular operand and the set of attributes across *all* operands is needlessly confusing.  (Note: only in the API, the implementation is a bit more sane.)   I really like your suggestion to rename the current AttributeSet to something like AttributeList and publishing the existing AttributeSetNode.   Once we've done that, we can update the bulk add/remove APIs to be sane.  The current semantics are an utter mess.</blockquote><div><br></div><div>Yep. AttributeLists shouldn't be a list of more AttributeLists. I've got a patch out which tries to untangle that a little bit more: <a href="https://reviews.llvm.org/D31198">https://reviews.llvm.org/D31198</a></div><div><br></div><div>With that patch, getParamAttributes(unsigned) returns an AttributeSetNode* instead of an AttributeList.</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"><span class="gmail-">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Optimizations<br>
=============<br>
<br>
Testing for presence of an attribute on a parameter should be fast. Today it is<br>
linear in the size of the prototype. I propose that we change AttributeList to<br>
make attributes randomly accessible by slot index, so that it stores a trailing<br>
object array of AttributeSetNode*, or some equivalent type that makes it<br>
efficient to test for attribute presence. I'll do some benchmarking to show that<br>
this change doesn't measurably impact LTO memory usage.<br>
</blockquote></span>
Sounds like a generally good idea.  Maybe we can hide the indexing from the client though?  Do something like store the index in the Value itself and have the AttributeSet (current name) interface be in terms of the Use?  Not a strong requirement or anything, just thinking that might keep the interface a bit cleaner.</blockquote><div><br></div><div>Huh, I hadn't considered building the API in terms of Use& or Use*. That would greatly reduce the need for all these extra integer induction variables and let us use a lot more range-based for loops. I think we can compute operand # from Use* in constant time, right? Thanks for the idea!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I also want to reduce the number of loads needed to test a single attribute.<br>
One potential optimization is to move the bitset of enum attributes up out of<br>
AttributeSetNode* into a wrapper type, perhaps called AttributeSet. The wrapper<br>
type could be a bitset and pointer pair, or we could try to union those together<br>
in the usual way to save memory. I suspect the memory savings of the union are<br>
unimportant, but I will measure. If we can always store the enumerated<br>
attributes that have no associated size locally, that seems ideal, since it<br>
allows modifying them without copying the immutable data stored in the LLVM<br>
context.<br>
</blockquote></span>
I got lost here.  Might be good to land some of the renames and then restate this.  I think we may be confusing meanings of AttributeSet?</blockquote><div><br></div><div>Yeah, I was trying to describe a *new* AttributeSet type. It would probably look something like:</div><div><br></div><div>class AttributeSet {</div><div>  uint64_t EnumAttrs; // bitset of that can answer hasAttr with a load and test</div><div>  AttributeSetNode *SlowAttrs; // string and integer attributes</div><div>...<br>};</div><div><br></div><div>This would be a trivially copyable value type, and would describe the attributes of a single function, arg, or ret. AttributeList would become an array of these. Adding an enum attribute is very cheap now: copy 16 (or 12) bytes of data and set a bit.</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"><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Another idea is to eliminate the use of AttributeList completely from Function,<br>
and instead store the attributes of an Argument directly *in* the Argument. The<br>
function and return attributes would live on the Function. At the end of the<br>
day, `Arg->hasNonNullAttr()` should compile to a load and a bittest. This is a<br>
more invasive change that would require more API migration, but again it would<br>
avoid more FoldingSet usage.<br>
</blockquote></span>
I particularly like this.  It's worth pointing out that we can do the same for return values of Invokes and Calls, but that handling parameter attributes is much harder.  This is also the point where I'd point out that metadata and return attributes have grown to heavily overlap in semantic with distinct sets of instructions. Maybe it's time to common the implementation, even if the interfaces stay separate?  Once we'd done that, we'd have three cases: return attributes/metadata on all instructions, parameter attributes on Call and Invoke, and Argument attributes on Functions.</blockquote><div><br></div><div>As nice as this is, I think I've convinced myself to stop short of doing this. It's nice if Function, CallInst, and InvokeInst all use the same data structures to store attributes. As long as Arg->hasAttr(Enum) is O(1), I think we've won. Moving the data out of the parallel array and into Argument probably isn't necessary.</div></div></div></div>