[IR] Add additional addParamAttr/removeParamAttr to AttributeList API
Needs ReviewPublic

Authored by dneilson on Fri, May 19, 6:59 AM.

Details

Summary

This is a NFC.

Fairly straightforward patch to fill in some of the holes in the attributes API with respect to accessing parameter/argument attributes. The patch aims to step further towards encapsulating the idx+FirstArgIndex pattern to access these attributes to within the AttributeList.

Diff Detail

dneilson created this revision.Fri, May 19, 6:59 AM
dneilson updated this revision to Diff 99607.Fri, May 19, 11:37 AM

clang-formatting patch

dneilson edited the summary of this revision. (Show Details)Fri, May 19, 11:46 AM
rnk added a comment.Fri, May 19, 2:52 PM

Thanks for working on this!

include/llvm/IR/Attributes.h
394

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.

lib/IR/Attributes.cpp
1023

If we can delete this version, we don't need the comment.

Thanks a lot for this. Some small comments -

include/llvm/IR/Attributes.h
418
  1. "returns a new set" => "returns a new list".
  1. 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..."
591

Should this comment be "exists at the arg number"?

dneilson added inline comments.Tue, May 23, 7:04 AM
include/llvm/IR/Attributes.h
394

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?

418

Thanks. I agree with the nitpick; I'll try to make the comments in AttributeList more uniform.

591

Probably, yeah. I'm fairly new to LLVM's codebase. Are args typically referred to by "argument number" or "argument index" ?

rnk added inline comments.Tue, May 23, 10:22 AM
include/llvm/IR/Attributes.h
394

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.

dneilson added inline comments.Tue, May 23, 10:46 AM
include/llvm/IR/Attributes.h
394

Ah, yes. That makes far more sense. It also gets rid of the code clone. So, it's win-win.