[PATCH] D32819: [IR] Switch AttributeList to use an array for O(1) access

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 5 inline comments as done.
rnk added a subscriber: rsmith.
rnk added a comment.

Thanks!



================
Comment at: llvm/lib/IR/AttributeImpl.h:235
 
   void operator delete(void *p) { ::operator delete(p); }
 
----------------
hans wrote:
> Not related to your patch, but why is this needed?
This appears to inhibit sized deallocation. @rsmith added it in r260180. Perhaps it deserves a comment.


================
Comment at: llvm/lib/IR/Attributes.cpp:813
+                "function should be stored in slot 0");
+  for (Attribute I : Sets.front()) {
+    if (!I.isStringAttribute())
----------------
hans wrote:
> I might have gone for `Sets[0]` to make it more obvious that's what the assert above is about. Or would it make sense to just do `Sets[attrIdxToArrayIdx(AttributeList::FunctionIndex)]`?
Perhaps, but the only reason that .front() or [0] are safe is because we asserted above that Sets isn't empty, so we still need some assertion or comment indicating that function attributes are always available. I'll do `Set[0]`.


================
Comment at: llvm/lib/IR/Attributes.cpp:957
+    // Drop the empty argument attribute sets at the end.
+    ArgAttrs = ArgAttrs.take_front(LastAttrPos - 1);
+    AttrSets.insert(AttrSets.end(), ArgAttrs.begin(), ArgAttrs.end());
----------------
hans wrote:
> I had to do a double-take on that -1, but it looks right.
> 
> I wonder if this function could be simpler though. Perhaps first loop from the back of ArgAttrs to find the last one that has attributes, then based on that and .hasAttributes on FnAttrs and RetAttrs, either return early or push into the SmallVector straight up?
I implemented this, and I also switched to a set count instead of a "last position" variable, which I think improves things a lot. I don't know why I used "LastPos" instead of a size or count variable. I think that is what was introducing these confusing off by one calculations.


================
Comment at: llvm/lib/IR/Attributes.cpp:969
+  Index = attrIdxToArrayIdx(Index);
+  SmallVector<AttributeSet, 5> AttrSets(Index + 1);
+  AttrSets[Index] = AttributeSet::get(C, B);
----------------
hans wrote:
> Is 5 significant?
Not really, let's go to 8 for consistency. That is one thing I hate about SmallVector: you are forced to invent magic numbers all the time, and it makes the reader think the number was tuned. :)


https://reviews.llvm.org/D32819