[PATCH] D81335: [IR] AttrBuilder: change TargetDepAttrs to StringMap<SmallString<16>>

[Benjamin Kramer via Phabricator via llvm-commits]

bkramer added inline comments.


================
Comment at: llvm/include/llvm/IR/Attributes.h:727
   std::bitset<Attribute::EndAttrKinds> Attrs;
-  std::map<std::string, std::string, std::less<>> TargetDepAttrs;
+  StringMap<SmallString<16>> TargetDepAttrs;
   MaybeAlign Alignment;
----------------
I'm a bit confused that SmallString gives an improvement in the number of allocations. All modern standard libraries implement std::string with SSO, so std::string should behave identically to SmallString<16>. What standard library did you test this with?


================
Comment at: llvm/include/llvm/IR/Attributes.h:871
   // Iterators for target-dependent attributes.
-  using td_type = std::pair<std::string, std::string>;
+  using td_type = std::pair<StringRef, StringRef>;
   using td_iterator = decltype(TargetDepAttrs)::iterator;
----------------
I don't think this is used anywhere. Can we just delete it?


================
Comment at: llvm/lib/IR/Attributes.cpp:881
 
+  llvm::sort(Attrs);
   return getSorted(C, Attrs);
----------------
Just sorting the string attributes would be sufficient. Sorting attributes is expensive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81335