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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Nice change - thanks!



================
Comment at: lib/IR/AttributeImpl.h:185
 
+  DenseMap<StringRef, Attribute> StringAttrs;
+
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78859