AttrBuilder API change

Hi folks,

the AttrBuilder class has been showing off a lot on performance report,
impacting compile time in a negative and unnecessery way. Several commits [1] by
Nikita Popov already improved the situation by avoid unnecesseray calls to this
builder, and more recenly, the AttributeMask class got introduced [2] as a way to
split the logic between adding and removing attributes.

The final step [3] (that sometimes give a 1% performance increase! [4]) avoid
using an std::map of SmallString to store target dependent attributes, and uses
a SmallVector of Attribute instead.

This implies changing all AttrBuilder constructors to take an LLVMContext as
parameter. Given how widely used this class is, I thought I'd advertise the
change here and let it mature a few extra days before it gets merged!

In [3], rnk is suggesting I'd land a temporary commit that introduces extra
constructors that accept an extra LLVM Context just to ease the transition.
Would that be useful to someone here?

[1] eg 6e30cb7673df293fd294acef7eadca8050b5a71e
[2] 9290ccc3c1a17a7874de020656db38183a20f6b0
[3] ⚙ D116599 Simplify AttrBuilder storage for target dependent attributes
[4] LLVM Compile-Time Tracker

+1 from me.

As an aside, has anyone looked at memoizing the strings used for string attributes? In practice, there tend to be a very small number of unique strings which are heavily reused. If we had a string table for the attribute strings, we could hold the index in the Attribute itself, and reduce storage fairly significantly. (Well, assuming we implemented the small string optimization to handle the case where the attribute name is shorted than the index size.)

We could either do this implicitly, or even expose it in the public API. (i.e. LLVMContext has a method along the lines of "memorizeAttributeName" which returns a spare slot in the Attributes enum.)

Philip

While folks are fiddling with attributes – maybe someone would like to consider allowing non-string-keyed attributes to have string values? It’s always seemed quite odd that core (not target-specific) attributes need to use string keys only because they take a string value.

+1 here