[PATCH] D78859: [IR] Use map for string attributes (NFC)

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: lib/IR/AttributeImpl.h:185
 
+  DenseMap<StringRef, Attribute> StringAttrs;
+
----------------
wenlei wrote:
> nikic wrote:
> > tejohnson wrote:
> > > nikic wrote:
> > > > arsenm wrote:
> > > > > Is StringMap any better?
> > > > I believe StringMap is only useful if the map needs to own the string. In this case the string is already owned by the Attribute. (StringMap allocates each map entry separately, as strings are variable-length.)
> > > > 
> > > > There's also CachedHashStringRef that can be used here, but I don't think that's useful either, because the map is immutable, so we do not need to worry about rehashing overhead.
> > > Won't this store the Attributes twice (once in the TrailingObjects base class and once in the DenseMap)? Would it be better to just use a StringMap here to subsume the TrailingObjects and not duplicate the Attributes? Or store a pointer to the const * value returned by the iterator?
> > `Attribute` is a pointer-like type, the actual attribute objects are allocated separately and globally uniqued. So we're only duplicating pointers here.
> > 
> > It would in theory be possible to remove string attributes from the trailing objects and only store them in the map, but I think this would cause many complications for little benefit, as AttributeSet is not particularly memory-critical. In particular this would make it hard to iterate over all attributes, and no longer provide a fixed ordering for attributes. Possibly it would cause issues with folding set profiling as well, I'm not sure whether that depends on the ordering or not.
> While StringMap would duplicate the underlying string, the DJB hash used by StringMap should be more efficient than the generic hash for StringRef - see `djbHash` vs `hash_combine_range_impl`. Given this is a hot spot, the speed/size trade-off may favor `StringMap`.
I gave StringMap a try, but the numbers are markedly worse: http://llvm-compile-time-tracker.com/compare.php?from=0cc4958dd2dda646abda6a7b01aeb6b50b601e6b&to=5655d81e5dd2e753248b47f26d09e86171f732d0&stat=instructions This is relative to using DenseMap of StringRef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78859