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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
