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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson 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;
----------------
rnk wrote:
> 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.
Ah, yes. That makes far more sense. It also gets rid of the code clone. So, it's win-win.


https://reviews.llvm.org/D33355