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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/lib/IR/AttributeImpl.h:235
 
   void operator delete(void *p) { ::operator delete(p); }
 
----------------
Not related to your patch, but why is this needed?


================
Comment at: llvm/lib/IR/Attributes.cpp:813
+                "function should be stored in slot 0");
+  for (Attribute I : Sets.front()) {
+    if (!I.isStringAttribute())
----------------
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)]`?


================
Comment at: llvm/lib/IR/Attributes.cpp:815
+    if (!I.isStringAttribute())
+      AvailableFunctionAttrs |= ((uint64_t)1) << I.getKindAsEnum();
   }
----------------
Not related to this patch of course, but is there any reason to not just use `1ULL`?


================
Comment at: llvm/lib/IR/Attributes.cpp:847
-    LLVMContext &C, ArrayRef<std::pair<unsigned, AttributeSet>> Attrs) {
-  assert(!Attrs.empty() && "creating pointless AttributeList");
-#ifndef NDEBUG
----------------
I suppose we'd still assert when hitting the AttributeListImpl constructor, but maybe this is worth keeping to assert early?


================
Comment at: llvm/lib/IR/Attributes.cpp:939
+  unsigned Pos = 2;
   for (AttributeSet AS : ArgAttrs) {
     if (AS.hasAttributes())
----------------
I think it would be more clear to loop over ArgAttrs with an index and just set `LastAttrPos = I + 2`. Or see my suggestion further down.


================
Comment at: llvm/lib/IR/Attributes.cpp:951
+  AttrSets.reserve(LastAttrPos + 1);
+  // If we have any attributes, we always have function attributes.
+  AttrSets.push_back(FnAttrs);
----------------
I didn't know that. Is it worth asserting?


================
Comment at: llvm/lib/IR/Attributes.cpp:954
+  if (LastAttrPos >= 1)
+    AttrSets.push_back(RetAttrs);
+  if (LastAttrPos >= 2) {
----------------
Are there always RetAttrs if there are ArgAttrs? Oh, it doesn't matter; pushing an empty RetAttrs does the job just as well.


================
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());
----------------
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?


================
Comment at: llvm/lib/IR/Attributes.cpp:969
+  Index = attrIdxToArrayIdx(Index);
+  SmallVector<AttributeSet, 5> AttrSets(Index + 1);
+  AttrSets[Index] = AttributeSet::get(C, B);
----------------
Is 5 significant?


================
Comment at: llvm/lib/IR/Attributes.cpp:998
+  unsigned MaxSize = 0;
+  for (auto &List : Attrs)
+    MaxSize = std::max(MaxSize, List.getNumAttrSets());
----------------
Below you just do `for (AttributeList List : Attrs`, i.e. no auto and no reference. That seems nicer.


================
Comment at: llvm/lib/IR/Attributes.cpp:1035
+  unsigned MaxIndex = Indices.back();
+  MaxIndex = attrIdxToArrayIdx(MaxIndex);
+  if (MaxIndex >= AttrSets.size())
----------------
Maybe fold into the previous statement? `unsigned MaxIndex = attrIdxToArrayIdx(Indices.back())`.


================
Comment at: llvm/lib/IR/Instructions.cpp:457
 
+  if (i == 0)
+    return hasRetAttr(Kind);
----------------
Use ReturnIndex instead of i?


https://reviews.llvm.org/D32819