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

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LG from my side, but I'd like a second opinion for the "require LLVMContext in AttrBuilder" part of the change, as that's the main API impact.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:136
     const std::vector<unsigned> &Attrs = RAG.second;
-    AttrBuilder B;
+    AttrBuilder B(M->getContext());
 
----------------
These `M->getContext()` can be replaced by just `Context`, as LLParser stores a reference to that.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1246
+  if (R == NumberedAttrBuilders.end())
+    R = NumberedAttrBuilders.emplace(VarID, AttrBuilder(M->getContext())).first;
+
----------------
Is the separate find call here to avoid constructing AttrBuilder if it's already in the map?

I was going to suggest that you can write this as `R = NumberedAttrBuilders.emplace(VarID, M->getContext()).first;` and leave the construction of `AttrBuilder` to emplace(), but it turns out that emplace() still unconditionally constructs the object (because C++) and the sensible replacement for this (`try_emplace`) has only been introduced in C++17.


================
Comment at: llvm/lib/IR/Attributes.cpp:1791
   for (const auto &I : AM.td_attrs())
-    TargetDepAttrs.erase(I);
+    removeAttribute(I);
 
----------------
We might as well directly resolve this TODO, I think it should be as simple as:
```
erase_if(TargetDepAttrs, [&](Attribute A) { return AM.contains(A); });
```


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

https://reviews.llvm.org/D116599