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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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;
----------------
bkramer wrote:
> 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?
I'm on debian sid, so there's really nothing weird going on here.
```
$ ldd bin/clang | grep c++
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fe8aa3a0000)
$ dpkg -S libstdc++.so
libx32stdc++6: /usr/libx32/libstdc++.so.6.0.28
libstdc++6:amd64: /usr/share/gdb/auto-load/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28-gdb.py
libx32stdc++6: /usr/libx32/libstdc++.so.6
libx32stdc++6: /usr/share/gdb/auto-load/usr/libx32/libstdc++.so.6.0.28-gdb.py
lib32stdc++6: /usr/lib32/libstdc++.so.6.0.28
libstdc++-10-dev:amd64: /usr/lib/gcc/x86_64-linux-gnu/10/libstdc++.so
libstdc++-8-dev:amd64: /usr/lib/gcc/x86_64-linux-gnu/8/libstdc++.so
libstdc++-9-dev:amd64: /usr/lib/gcc/x86_64-linux-gnu/9/libstdc++.so
libstdc++6:amd64: /usr/lib/x86_64-linux-gnu/libstdc++.so.6
lib32stdc++6: /usr/lib32/libstdc++.so.6
lib32stdc++6: /usr/share/gdb/auto-load/usr/lib32/libstdc++.so.6.0.28-gdb.py
libstdc++6:amd64: /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28
$ apt-cache show libstdc++6:amd64
Package: libstdc++6
Source: gcc-10
Version: 10.1.0-3
Installed-Size: 2351
Maintainer: Debian GCC Maintainers <debian-gcc at lists.debian.org>
Architecture: amd64
Replaces: libstdc++6-10-dbg (<< 4.9.0-3)
Depends: gcc-10-base (= 10.1.0-3), libc6 (>= 2.18), libgcc-s1 (>= 4.2)
Conflicts: scim (<< 1.4.2-1)
Breaks: gcc-4.3 (<< 4.3.6-1), gcc-4.4 (<< 4.4.6-4), gcc-4.5 (<< 4.5.3-2)
Description-en: GNU Standard C++ Library v3
 This package contains an additional runtime library for C++ programs
 built with the GNU compiler.
 .
 libstdc++-v3 is a complete rewrite from the previous libstdc++-v2, which
 was included up to g++-2.95. The first version of libstdc++-v3 appeared
 in g++-3.0.
Description-md5: 724ab84919e0e220afb960e36463914d
Multi-Arch: same
Homepage: http://gcc.gnu.org/
Tag: role::shared-lib
Section: libs
Priority: optional
Filename: pool/main/g/gcc-10/libstdc++6_10.1.0-3_amd64.deb
Size: 491824
MD5sum: b3c01846fda0d56a0ec15e24935a0da6
SHA256: 7349532219fbc71a211954dc0a7ca136343c14769cb71c9c4cabe032d6cc9c38
```

> All modern standard libraries implement std::string with SSO, so std::string should behave identically to SmallString<16>.

I see:
```
      enum { _S_local_capacity = 15 / sizeof(_CharT) };

      union
      {
	_CharT           _M_local_buf[_S_local_capacity + 1];
	size_type        _M_allocated_capacity;
      };
```
So i would say it's one `char` less.
Also, here
```
  static_assert(sizeof(SmallString<16>) == 40);
  static_assert(sizeof(std::string) == 32);
```
which means that given same buffer size, we'll fit less `SmallString`s in it,
causing new allocation sooner. How does `StringMap` grow?
I'd guess by consuming memory more aggressively, we end up growing less times.
At least that would be my guess.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81335