This is an archive of the discontinued LLVM Phabricator instance.

Avoid using AttrBuilder as an intermediate step when not needed
AbandonedPublic

Authored by serge-sans-paille on Dec 21 2021, 7:49 AM.

Details

Reviewers
nikic
Summary

AttrBuilder is an helpful but costly data structure; it is not
needed to construct one when a single element is added.

Using an explicit constructor for AttrBuilder helps spotting cases where an
implicit conversion happens when the appropriate method can be used instead.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Dec 21 2021, 7:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2021, 7:49 AM

rebase on main branch

nikic added a comment.EditedJan 15 2022, 6:04 AM

I'm not a fan of this change, because it duplicates logic and introduces a good number of additional overloads, for not a lot of practical benefit on compile-time. I think our general goal should be to make AttrBuilder fast enough that we do not need to add specialized implementations for specific implementations.

If we change AttrBuilder to just be an API around SmallVector<Attribute> we get some nice improvements: https://llvm-compile-time-tracker.com/compare.php?from=64590312d4b84e13345a98b9d7d7bbc4e2b0c166&to=1ba9059e40345e3530dcb0af9b6bda049f4aab8f&stat=instructions At that point, doing an unnecessary AttributeSet -> AttrBuilder conversion only involves copying attributes into a different vector, which is cheap enough that it probably does not warrant further optimization anymore. I also checked what happens if I combine that with the optimized merge implementation you added here, but it doesn't seem to make a measurable difference (https://llvm-compile-time-tracker.com/compare.php?from=1ba9059e40345e3530dcb0af9b6bda049f4aab8f&to=6890ee94894d93b744514a73e8b592e8284a5f2f&stat=instructions). My guess is that the right hand side AttrBuilder tends to only contain very few attributes, so doing individual lower_bound insertions works well enough.

Unfortunately, I discovered a problem while looking into this: Apparently, we don't have a well-defined behavior when adding an attribute that already exists (same key, different value). We have a mix of different behaviors:

I think before we optimize attribute addition, we first need to decide on a single behavior. I think it would either make sense to pick the one you implemented here (i.e. attribute from RHS wins) or to forbid adding an attribute with the same key (unless it also has same value or no value). In the latter case the user would be required to remove the old attribute beforehand.

serge-sans-paille abandoned this revision.Feb 9 2022, 4:38 AM