llvm dev - Jan 2022 - 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] https://reviews.llvm.org/D116599
[4]
https://llvm-compile-time-tracker.com/compare.php?from=5de322295f4ade692dc4f1823ae4450ad3c48af2&to=05bc480bf641a9e3b466619af43a2d123ee3f71d&stat=instructions

+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

On 1/6/22 7:00 AM, Serge Guelton via llvm-dev wrote:
> 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] https://reviews.llvm.org/D116599
> [4]
https://llvm-compile-time-tracker.com/compare.php?from=5de322295f4ade692dc4f1823ae4450ad3c48af2&to=05bc480bf641a9e3b466619af43a2d123ee3f71d&stat=instructions
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev