[PATCH] D33355: [IR] Add additional addParamAttr/removeParamAttr to AttributeList API

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: include/llvm/IR/Attributes.h:394
   /// attribute sets are immutable, this returns a new set.
   AttributeList addAttribute(LLVMContext &C, ArrayRef<unsigned> Indices,
                              Attribute A) const;
----------------
dneilson wrote:
> rnk wrote:
> > I'd like to remove this overload now that it is dead within LLVM. You may need to replace it with one that takes a single Index.
> This overload appears to be used in both InstCombineCalls.cpp and CorrelatedValuePropagation.cpp (see the two changed files at the end of this patch). If I remove these overloads, then I have to rewrite those bits of code to call addAttribute() directly, instead of push_back-ing the index into a list. Seems like a reasonable change to me. Do you concur?
I'm suggesting that you keep `addParamAttributes(LLVMContext&, ArrayRef<unsigned>, Attribute A)` but delete `addAttribute(LLVMContext&, ArrayRef<unsigned>, Attribute A)`. Does that make sense?

Every call to `addAttribute` effectively leaks an AttributeListImpl. This code is attempting to carefully avoid making extra AttributeListImpl memory allocations, and I would like to keep that carefulness, even if not all passes are doing it. Especially because we can infer `nonnull` about so many call sites. The other attribute inference stuff doesn't fire nearly as often.


https://reviews.llvm.org/D33355