[PATCH] D116111: Avoid using AttrBuilder as an intermediate step when not needed

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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=349006b452382ed1bc88a606e5b8c0e0e1f4f445&to=f0a08ac43042a3d8d25e7a7020a9aac7fe696d78&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=f0a08ac43042a3d8d25e7a7020a9aac7fe696d78&to=173b690dd3417a2559493d248f2f795acad79ea6&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:

- String attributes take their value from the RHS when merging.
- Int and type attributes take their value from the LHS when merging (https://github.com/llvm/llvm-project/blob/3ba96cb2c94904ab68a59cfd2391846f1e9b9902/llvm/lib/IR/Attributes.cpp#L1760-L1767).
- We have one place that asserts that overwriting with different alignment is not allowed, but other places that don't assert this (https://github.com/llvm/llvm-project/blob/3ba96cb2c94904ab68a59cfd2391846f1e9b9902/llvm/lib/IR/Attributes.cpp#L1253).
- And then to add insult to injury, `AttributeSet::addAttributes()` performs the merging the wrong way around (https://github.com/llvm/llvm-project/blob/3ba96cb2c94904ab68a59cfd2391846f1e9b9902/llvm/lib/IR/Attributes.cpp#L630-L632), so the behavior is the opposite from what `AttributeList::addAttributesAtIndex()` (the more commonly used API) does.

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.


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

https://reviews.llvm.org/D116111