[PATCH] D116599: Simplify AttrBuilder storage for target dependent attributes

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I'm generally on board with this change. After D116110 <https://reviews.llvm.org/D116110>, the `AttrBuilder` is only used in situations where we actually expect all the added attributes to be converted to `Attribute`s in any case, so we don't lose anything by storing them directly in that form. At the same time, we don't need to do expensive conversions from `Attribute` to internal `AttrBuilder` representation and back to `Attribute` (involving FoldingSet lookups) when `AttrBuilder` is constructed from an existing `AttributeList`/`AttributeSet`. This is common, because the general approach to attribute modification is "dump everything into AttrBuilder, add new attributes, convert back to AttributeSet".

The main externality here is that AttrBuilder now requires an LLVMContext, but it doesn't look like obtaining one is a problem for any existing usage. I do think it would be good to land the `LLVMContext` constructor argument addition in a separate patch from the internal representation change, as that one is essentially just busywork.

I've left some implementation nits...



================
Comment at: llvm/include/llvm/IR/Attributes.h:1006
+    }
+  };
+
----------------
Don't think this needs to be in the header.


================
Comment at: llvm/include/llvm/IR/Attributes.h:1008
+
+  LLVMContext& Ctx;
   std::bitset<Attribute::EndAttrKinds> Attrs;
----------------
`LLVMContext &Ctx` here and elsewhere.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1242
       parseToken(lltok::lbrace, "expected '{' here") ||
-      parseFnAttributeValuePairs(NumberedAttrBuilders[VarID], unused, true,
+      parseFnAttributeValuePairs(NumberedAttrBuilders.emplace(VarID, AttrBuilder(M->getContext())).first->second, unused, true,
                                  BuiltinLoc) ||
----------------
I'd split the if here and store the AttrBuilder in a variable and then reuse below. Getting a bit hard to read.


================
Comment at: llvm/lib/IR/Attributes.cpp:1612
+  return addAttribute(Attribute::get(Ctx, A, V));
+}
+
----------------
This method move looks a bit weird, I'd leave it where it was so we don't mix add/remove.


================
Comment at: llvm/lib/IR/Attributes.cpp:1761
 
+  // TODO: Inefficient...
   for (const auto &I : B.td_attrs())
----------------
Could be more specific here, e.g. `TODO: Could merge both lists in one loop`.


================
Comment at: llvm/lib/IR/Attributes.cpp:1769
 AttrBuilder &AttrBuilder::remove(const AttributeMask &AM) {
   // FIXME: What if both have an int/type attribute, but they don't match?!
   for (unsigned Index = 0; Index < Attribute::NumIntAttrKinds; ++Index)
----------------
Unrelated to this change, but with AttributeMask this FIXME no longer makes sense.


================
Comment at: llvm/lib/IR/Attributes.cpp:1833
+  return IntAttrs == B.IntAttrs && TypeAttrs == B.TypeAttrs &&
+         TargetDepAttrs == B.TargetDepAttrs;
 }
----------------
I'd combine the `Attrs == B.Attrs` check into this return as well, so it doesn't feel lonely :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116599/new/

https://reviews.llvm.org/D116599