This is an archive of the discontinued LLVM Phabricator instance.

Provide SmallAttrBuilder as a lightweight alternative to AttrBuilder
AbandonedPublic

Authored by serge-sans-paille on Dec 15 2021, 5:25 AM.

Details

Reviewers
nikic
dblaikie
rnk
Summary

This class can only add / remove Attributes, but it does it relatively
efficiently compared to existing AttrBuilder, providing a consistent compile-time
speedup according to https://llvm-compile-time-tracker.com/

Internally it maintains two SmallVector of sorted Attributes, which turns out to
be more efficient than temporary hashmap and bitfields as used by AttrBuilder.

Diff Detail

Unit TestsFailed

Event Timeline

serge-sans-paille requested review of this revision.Dec 15 2021, 5:25 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 15 2021, 5:25 AM

My plan mid-term plan would be to rename AttrBuilder into AttrQuery at some point, and use SmallAttrBuilder as the actual AttrBuilder in most places.

Leverage the fact that AttributeSet nodes are already sorted

nikic added a comment.Dec 15 2021, 6:25 AM

I like the general direction. A possible reframing would be SmallAttrBuilder -> MutableAttributeSet. I think my main question here would be in which contexts we still use / want to use the old AttrBuilder?

llvm/include/llvm/IR/Attributes.h
974

Just wondering if storing both in one vector would make sense? Would allow reusing the normal comparison on attributes and generally simplify the code. Or is that noticeably less efficient?

1125

What is uniquify() supposed to mean in this context?

llvm/include/llvm/IR/Attributes.h
974

Yeah, a single container is less efficient: It makes comparison more costly (as you need to check which kind of attribute we're dealing with each time), having two containers also reduces the cost of insert / remove (for low number of elements, we have higher chance to hit the container back). The lower bound alos typically takes less steps when the nature of attribute is known at compile time, which happens a lot.

1125

Woops that's a leftover of a previous attempt when I was not trying to maintain the sorted invariant, I'll rename this.

rnk added a comment.Dec 15 2021, 9:14 AM

I would really prefer to avoid adding a new variant of AttrBuilder. What is the main blocker to making AttrBuilder more efficient? It just needs an LLVMContext, right? Would that be feasible instead? Most AttrBuilders are constructed from existing AttributeLists, which have a context.

Internally it maintains two SmallVector of sorted Attributes, which turns out to
be more efficient than temporary hashmap and bitfields as used by AttrBuilder.

Surely the bitset is more efficient than a vector of attributes. I think it's the std::map<SmallString, SmallString> that's slow. I think replacing that with a sorted vector of hashed string attributes would be just as efficient.

I agree that we really should only have one attribute builder class.

A SmallVector does seem nicer than having a static array the size of all possible attributes. We should avoid creating copies of AttributeLists/Sets into AttrBuilders and just have AttrBuilder be a list of attribute diffs, basically what you have as SmallAttrBuilder, although I'm not sure if we also want to handle a list of attributes we want to *remove* from an AttributeSet in SmallAttrBuilder as well?

As for querying the AttrBuilder for attributes it contains, IMO we shouldn't need to in most cases, typically we can directly query the AttributeList/Set directly. https://reviews.llvm.org/D115819 shows that a bunch of the query methods in AttrBuilder are easily removed. LLParser::validateEndOfModule() is the last major user of AttributeList functionality we may not want to support but it seems fixable.