[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:
> 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?


================
Comment at: include/llvm/IR/Attributes.h:418
+  /// Add an attribute to the attribute set at the given arg indices. Because
+  /// attribute sets are immutable, this returns a new set.
+  AttributeList addParamAttribute(LLVMContext &C, ArrayRef<unsigned> ArgNos,
----------------
javed.absar wrote:
> 1. "returns a new set" => "returns a new list".
> 
> 2. Nitpick - it would be nice to make all such sentences same, some places we seem to start with "Returns a .. because.." other places as "Because.... returns..."
Thanks. I agree with the nitpick; I'll try to make the comments in AttributeList more uniform.


================
Comment at: include/llvm/IR/Attributes.h:591
 
+  /// \brief Return the attribute object that exists at the arg index.
+  Attribute getParamAttr(unsigned ArgNo, Attribute::AttrKind Kind) const {
----------------
javed.absar wrote:
> Should this comment be "exists at the arg number"?
Probably, yeah. I'm fairly new to LLVM's codebase. Are args typically referred to by "argument number" or "argument index" ?


https://reviews.llvm.org/D33355