[llvm-reduce] Reducing attributes

[lebedev.ri]

Summary

This handles all three places where attributes could currently be - GlobalVariable, Function and CallBase.
For last two, it correctly handles all three possible attribute locations (return value, arguments and function itself)

There was a previous attempt at it D73853,
which was committed in rGfc62b36a000681c01e993242b583c5ec4ab48a3c,
but then reverted all the way back in rGb12176d2aafa0ccb2585aa218fc3b454ba84f2a9
due to some (osx?) test failures.

[arsenm]

One of the problems with bugpoint's attribute handling is it burns a lot of time trying to reduce attributes on intrinsics, which cannot actually be removed since getDeclaration will just put them back on

llvm/tools/llvm-reduce/DeltaManager.h

36

Doing this last is an improvement over bugpoint's attempt to do this first.

I don't think removing attributes is actually a great reduction strategy. For most of the hard to reduce testcases I debug, removing attributes is entirely pointless (and adding them is more helpful). I think this needs a flag to disable it.

[nickdesaulniers]

I think it would be good in the description+commit message to reference https://reviews.llvm.org/D73853 (and that it was reverted).

Does llvm-reduce still have "interestingness tests" in llvm/test/Reduce/Inputs/? If so, shouldn't this change add such a test?

Thanks for the patch!

llvm/tools/llvm-reduce/DeltaManager.h

36

Counterpoint, I find removing attributes very helpful in reducing the amount of noise in reduced test cases and have had bugs when I needed to figure out which attribute was the source of differences in codegen.

I don't mind a flag (I don't think it's necessary, but doesn't hurt); but I'd prefer it to be default on so you can opt-out if you don't want to reduce attributes.

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

147–149

does zip actually simplify this sequence? Looks kind of complicated.

178

I wish all these members of AttributeRemapper were private and these three loops maybe hidden in a method of AttributeRemapper.

[lebedev.ri]

In D83351#2137603, @nickdesaulniers wrote:

I think it would be good in the description+commit message to reference https://reviews.llvm.org/D73853 (and that it was reverted).

Does llvm-reduce still have "interestingness tests" in llvm/test/Reduce/Inputs/? If so, shouldn't this change add such a test?

*cough* llvm/test/Reduce/*.ll
There are (no more new!) llvm/test/Reduce/Inputs/* files because they are pointless.
As you can see we can just as easily use FileCheck to drive interestingness tests.

Thanks for the patch!

[nickdesaulniers]

In D83351#2137610, @lebedev.ri wrote:

In D83351#2137603, @nickdesaulniers wrote:

I think it would be good in the description+commit message to reference https://reviews.llvm.org/D73853 (and that it was reverted).

Does llvm-reduce still have "interestingness tests" in llvm/test/Reduce/Inputs/? If so, shouldn't this change add such a test?

*cough* llvm/test/Reduce/*.ll
There are (no more new!) llvm/test/Reduce/Inputs/* files because they are pointless.
As you can see we can just as easily use FileCheck to drive interestingness tests.

Might be worth proving it if you're bored and doing other janitorial work in the area. Having homogeneous tests is nice, since we'd rather future travelers use FileCheck for BOTH interesting-ness and functional testing of their reduction pass itself.

[MaskRay]

llvm/test/Reduce/remove-call-site-attributes.ll

4

rm -f %t is sufficient.

21

Add -SAME: if applicable

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

156

If it is non-deterministic, use stable_sort

[dblaikie]

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

61–62

range-based-for loop, probably?

108–109

drop this since it's implicit/default?

147–149

+1 to this.

std::vector<std::pair<unsigned, AttributeSet>> Res;
Res.reserve(AttributeSets.size());
for (const auto &V : AttributeSets)
  Res.push_back({V.first, convertAttributeRefToAttributeSet(C, V.second)});

Seems simpler.

153–155

Could roll this into the previous for loop, making the push_back conditional:

std::vector<std::pair<unsigned, AttributeSet>> Res;
Res.reserve(AttributeSets.size());
for (const auto &V : AttributeSets) {
  AttributeSet S = convertAttributeRefToAttributeSet(C, V.second);
  if (S.hasAttributes())
    Res.push_back({V.first, S}); // maybe std::move(S)? Not sure if it's sufficiently heavy to benefit from moving
}

170

why std::for_each rather than a range-based for loop?

[lebedev.ri]

Thanks for taking a look!
Applied some refactoring, addressed some comments.

I will split some of the changes into separate commits when actually landing this.

llvm/tools/llvm-reduce/DeltaManager.h

36

We currently have no such options.
How about we deal with that afterwards, by just consistently adding one for each?

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

61–62

Hm, can use either. Why not for_each ?

147–149

After some refactoring...

156

It is.

170

why range-based for loop rather than a std::for_each?

These cases i'll prefer to keep as for_each, because it really doesn't matter in which order we iterate here,
while in range-based for loop is more for traditional direct forward iteration.

178

The delta pass consists of three stages:

1. just counting all the features
2. enumerating each feature (in a stable order) and recording whether or not it is to be kept
3. actually applying the rewrite from the previous step

Right now, i think each step is neatly separated into AttributeCounter,
AttributeRemapper and extractAttributesFromModule.
I think, sinking implementation detail of the last step into middle step will convolute things.

[lebedev.ri]

IWYU

[nickdesaulniers]

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

61–62

I find the range-for more concise, FWIW.

69–71

if (AS.getNumAttributes())
  visitAttributeSet(AS, GlobalVariablesToRefine[&GV]);

[lebedev.ri]

Addressing review nit.

[nickdesaulniers]

I'm not a fan of the inconsistent use of range-for and for-each; I would prefer range-for everywhere since it's more concise.

But I don't feel strongly enough to block the patch based on that. Maybe LLVM's style guide should provide clarity and guidance on the difference of opinion?

It might be polite to see if other reviewers have additional and timely feedback.

[lebedev.ri]

In D83351#2139586, @nickdesaulniers wrote:

I'm not a fan of the inconsistent use of range-for and for-each; I would prefer range-for everywhere since it's more concise.

My (consistently inconsistent) headcanon as to which to use when is that for_each should be used
when in principle we don't care in which order each item will be processed.

But I don't feel strongly enough to block the patch based on that. Maybe LLVM's style guide should provide clarity and guidance on the difference of opinion?

It might be polite to see if other reviewers have additional and timely feedback.

Sure, let's see if @dblaikie / etc have any other comments.

[dblaikie]

In D83351#2139599, @lebedev.ri wrote:

In D83351#2139586, @nickdesaulniers wrote:

I'm not a fan of the inconsistent use of range-for and for-each; I would prefer range-for everywhere since it's more concise.

My (consistently inconsistent) headcanon as to which to use when is that for_each should be used
when in principle we don't care in which order each item will be processed.

I don't think that's how the rest of LLVM is written, nor probably a great model. std::for_each is guaranteed to visit the elements in order, so it doesn't have a different contract to a range-based-for loop & adds some extra syntax (the lambda introducers, etc), complications to error messages, etc.

But I don't feel strongly enough to block the patch based on that. Maybe LLVM's style guide should provide clarity and guidance on the difference of opinion?

I think this is one I'm willing to say LLVM convention's pretty clear - there's 30 calls to std::for_each across LLVM and subprojects - and about 25,000 range-based-for loops... - so please change these to range based for loops.

[lebedev.ri]

There's also llvm::for_each() with 40 uses.
Can you please quote specific part of the whatever documentation you believe dictates this?
If there isn't one, i'd like to see ProgrammersManual patch.

[dblaikie]

In D83351#2139751, @lebedev.ri wrote:

There's also llvm::for_each() with 40 uses.

Still a very tiny fraction of all iterations. I expect most (or at least an order of magnitude or two more than the <100 for_each()s) of those iterations don't depend on the order of iteration - so I don't think it provides the signal you're thinking of, at least not to enough other developers to be useful - making the code quirky/different in a way that I think is likely to be confusing to other readers ("Why isn't this a range-based for loop? is there some subtle difference in behavior that the author intended that I'm not understanding?" & there isn't)

Can you please quote specific part of the whatever documentation you believe dictates this?
If there isn't one, i'd like to see ProgrammersManual patch.

Perhaps this is sufficient: https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible

[lebedev.ri]

In D83351#2139769, @dblaikie wrote:

In D83351#2139751, @lebedev.ri wrote:

There's also llvm::for_each() with 40 uses.

Still a very tiny fraction of all iterations. I expect most (or at least an order of magnitude or two more than the <100 for_each()s) of those iterations don't depend on the order of iteration - so I don't think it provides the signal you're thinking of, at least not to enough other developers to be useful - making the code quirky/different in a way that I think is likely to be confusing to other readers ("Why isn't this a range-based for loop? is there some subtle difference in behavior that the author intended that I'm not understanding?" & there isn't)

Can you please quote specific part of the whatever documentation you believe dictates this?
If there isn't one, i'd like to see ProgrammersManual patch.

Perhaps this is sufficient: https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible

I'm afraid it is not. It only speaks about range-based for loop vs. old-style for loops.
If for_each is so bad, we really should proactively document it, so i'm still waiting on the link/patch.
Thanks.

[arsenm]

This should skip trying to remove attributes from intrinsics

[dblaikie]

In D83351#2139818, @lebedev.ri wrote:

In D83351#2139769, @dblaikie wrote:

In D83351#2139751, @lebedev.ri wrote:

There's also llvm::for_each() with 40 uses.

Still a very tiny fraction of all iterations. I expect most (or at least an order of magnitude or two more than the <100 for_each()s) of those iterations don't depend on the order of iteration - so I don't think it provides the signal you're thinking of, at least not to enough other developers to be useful - making the code quirky/different in a way that I think is likely to be confusing to other readers ("Why isn't this a range-based for loop? is there some subtle difference in behavior that the author intended that I'm not understanding?" & there isn't)

Can you please quote specific part of the whatever documentation you believe dictates this?
If there isn't one, i'd like to see ProgrammersManual patch.

Perhaps this is sufficient: https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible

I'm afraid it is not. It only speaks about range-based for loop vs. old-style for loops.
If for_each is so bad, we really should proactively document it, so i'm still waiting on the link/patch.
Thanks.

I don't think this is an LLVM-specific C++ stylistic issue & there's lots of ways that we could write less than ideal code that we don't document in the style guide. (general guidance like "don't use dynamic allocation when static allocation will do" for instance) & I think this fits under that kind of category.

The existing uses of llvm::for_each look like mostly cases where an existing lambda is used in a few different places and it's slightly shorter/easier to use std::for_each than a range-based-for. I think those are OK/wouldn't object to them. But in these cases where a lambda expression is being passed immediately to for_each, it doesn't seem beneficial to me.

[nickdesaulniers]

In D83351#2139818, @lebedev.ri wrote:

so i'm still waiting on the link/patch.

Treat your fellow contributors with more respect, please.  I know style disagreements aren't exciting, but we're all on the same team.  I much prefer LLVM's community to the Linux kernel's for a reason, and I think it's worthwhile to speak up in defense of it, lest it decay.  I'm not the best at maintaining this myself, so if you see me break my own standards, please feel empowered to call me out.

Another benefit of range for here is we don't need the braces, so these can be 2 lines instead of 3. Please change them to be consistent.

[lebedev.ri]

Addressing nits:

• It turns out, we can't add attributes to true intrinsics, so indeed, there is no point in trying to reduce them. Originally, i thought that wasn't the case and didn't want to deal with all that (with having to understand which attribute will just reappear if it is deleted) right away. So just ignore true intrinsics.
• for_each changes

I will split this up into several commits when landing.

In D83351#2139879, @nickdesaulniers wrote:

In D83351#2139818, @lebedev.ri wrote:

so i'm still waiting on the link/patch.

Treat your fellow contributors with more respect, please.  I know style disagreements aren't exciting, but we're all on the same team.

Please remember that any [self-respecting] community is diverse, and almost by definition
that includes people with different 'base' languages. American speak of sugar-coating
very message is neither universally-used nor is required,
and not always following it does not mean ill-intent.

I much prefer LLVM's community to the Linux kernel's for a reason, and I think it's worthwhile to speak up in defense of it, lest it decay.  I'm not the best at maintaining this myself, so if you see me break my own standards, please feel empowered to call me out.

Another benefit of range for here is we don't need the braces, so these can be 2 lines instead of 3. Please change them to be consistent.

[nickdesaulniers]

In D83351#2140113, @lebedev.ri wrote:

I will split this up into several commits when landing.

What's the plan?

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

75–77

// We can neither add nor remove attributes from intrinsics.
if (F.getIntrinsicID() == Intrinsic::not_intrinsic)
  visitAttributeList(F.getAttributes(), FunctionsToRefine[&F]);

[lebedev.ri]

In D83351#2140177, @nickdesaulniers wrote:

In D83351#2140113, @lebedev.ri wrote:

I will split this up into several commits when landing.

What's the plan?

langref(?), test cleanup, for_each cleanup, rest of the patch

[lebedev.ri]

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

75–77

We can do that, with no line count diff and doesn't the comment loos context - right now it's accurately on the early-return?

[nickdesaulniers]

In D83351#2140189, @lebedev.ri wrote:

In D83351#2140177, @nickdesaulniers wrote:

In D83351#2140113, @lebedev.ri wrote:

I will split this up into several commits when landing.

What's the plan?

langref(?), test cleanup, for_each cleanup, rest of the patch

SGTM, if you want to split those off in child revisions/phab reviews I'd be happy to review/approve test cleanup, for_each, and the rebased patch for this feature.

I think you meant coding standard (as opposed to langref)? (Just checking, or was there a LangRef change, too, and I missed it?)

[lebedev.ri]

In D83351#2140238, @nickdesaulniers wrote:

In D83351#2140189, @lebedev.ri wrote:

In D83351#2140177, @nickdesaulniers wrote:

In D83351#2140113, @lebedev.ri wrote:

I will split this up into several commits when landing.

What's the plan?

langref(?), test cleanup, for_each cleanup, rest of the patch

SGTM, if you want to split those off in child revisions/phab reviews I'd be happy to review/approve test cleanup, for_each, and the rebased patch for this feature.

Since that wasn't paired with an accept, i can only surmise that it wasn’t an offer.

[MaskRay]

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

[dblaikie]

In D83351#2139879, @nickdesaulniers wrote:

In D83351#2139818, @lebedev.ri wrote:

so i'm still waiting on the link/patch.

Treat your fellow contributors with more respect, please.  I know style disagreements aren't exciting, but we're all on the same team.

Please remember that any [self-respecting] community is diverse, and almost by definition
that includes people with different 'base' languages. American speak of sugar-coating
very message is neither universally-used nor is required,
and not always following it does not mean ill-intent.

Sure enough - though the response did feel rather dismissive to me. I was explaining why I felt that the stylistic guidance was merited, but also wasn't what I think is best for the style guide (I think there's a lot of things that would be considered less readable that we can't encode in the style guide due to there being too many of them - that matter doesn't seem to have been answered/engaged with) and your response seemed to ignore the merits of that and abruptly repeat your point.

It's important for diversity to also create a welcoming community - that's not necessarily sugar coating, but engaging with kindness and understanding to other folks & hopefully be able to engage with the discussion in productive ways. None of us get this perfect all the time, to be sure.

[gchatelet]

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

87

@lebedev.ri I was updating seq in D103900 and an assertion fired at this location because seq is being called with begin being greater than end
seq(AL.index_begin(), AL.index_end()) => seq(4294967295, 3)

I'm not familiar with this code but it seems that in the context of function attributes begin_index and end_index have special meaning (See here and here).

Can you have a look?

[lebedev.ri]

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

87

It probably should be an integer, not an unsigned then.

llvm/test/Reduce/remove-attributes-from-intrinsic-like-functions.ll

+; Just because a function is named like an intrinsic does not mean we should skip it's attributes.
+;
+; RUN: llvm-reduce --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: cat %t | FileCheck --check-prefixes=CHECK-ALL,CHECK-FINAL %s
+
+; CHECK-ALL: declare i32 @llvm.not.really.an.intrinsic(i32, i32) #0
+declare i32 @llvm.not.really.an.intrinsic(i32, i32) #0
+
+define i32 @t(i32 %a) {
+; CHECK-ALL-LABEL: @t(
+
+; CHECK-INTERESTINGNESS: %r =
+; CHECK-INTERESTINGNESS-SAME: call
+; CHECK-INTERESTINGNESS-SAME: "arg0"
+; CHECK-INTERESTINGNESS-SAME: i32 @llvm.not.really.an.intrinsic(i32
+; CHECK-INTERESTINGNESS-SAME: "arg3"
+; CHECK-INTERESTINGNESS-SAME: %a
+; CHECK-INTERESTINGNESS-SAME: i32
+; CHECK-INTERESTINGNESS-SAME: %a
+; CHECK-INTERESTINGNESS-SAME: #1
+
+; CHECK-FINAL: %r = call "arg0" i32 @llvm.not.really.an.intrinsic(i32 "arg3" %a, i32 %a) #1
+; CHECK-ALL: ret i32 %r
+
+  %r = call "arg0" "arg1" i32 @llvm.not.really.an.intrinsic(i32 "arg2" "arg3" %a, i32 %a) "arg4" "arg5"
+  ret i32 %r
+}
+
+; CHECK-INTERESTINGNESS: attributes #0 = {
+; CHECK-INTERESTINGNESS-SAME: "arg6"
+
+; CHECK-INTERESTINGNESS: attributes #1 = {
+; CHECK-INTERESTINGNESS-SAME: "arg4"
+
+; CHECK-FINAL: attributes #0 = { "arg6" }
+; CHECK-FINAL: attributes #1 = { "arg4" }
+
+; CHECK-ALL-NOT: attributes #
+
+attributes #0 = { "arg6" "arg7" }

llvm/test/Reduce/remove-attributes-from-intrinsics.ll

+; We can't actually put attributes on intrinsic declarations, only on call sites.
+;
+; RUN: llvm-reduce --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: cat %t | FileCheck --check-prefixes=CHECK-ALL,CHECK-FINAL %s
+
+define i32 @t(i32 %a) {
+; CHECK-ALL-LABEL: @t(
+
+; CHECK-INTERESTINGNESS: %r =
+; CHECK-INTERESTINGNESS-SAME: call
+; CHECK-INTERESTINGNESS-SAME: "arg0"
+; CHECK-INTERESTINGNESS-SAME: i32 @llvm.uadd.sat.i32(i32
+; CHECK-INTERESTINGNESS-SAME: "arg3"
+; CHECK-INTERESTINGNESS-SAME: %a
+; CHECK-INTERESTINGNESS-SAME: i32
+; CHECK-INTERESTINGNESS-SAME: %a
+; CHECK-INTERESTINGNESS-SAME: #1
+
+; CHECK-FINAL: %r = call "arg0" i32 @llvm.uadd.sat.i32(i32 "arg3" %a, i32 %a) #1
+; CHECK-ALL: ret i32 %r
+
+  %r = call "arg0" "arg1" i32 @llvm.uadd.sat.i32(i32 "arg2" "arg3" %a, i32 %a) "arg4" "arg5"
+  ret i32 %r
+}
+
+; CHECK-ALL: declare i32 @llvm.uadd.sat.i32(i32, i32) #0
+declare i32 @llvm.uadd.sat.i32(i32, i32) #0
+
+; CHECK-ALL: attributes #0 = { nounwind readnone speculatable willreturn }
+
+; CHECK-INTERESTINGNESS: attributes #1 = {
+; CHECK-INTERESTINGNESS-SAME: "arg4"
+
+; CHECK-FINAL: attributes #1 = { "arg4" }
+
+; CHECK-ALL-NOT: attributes #
+
+attributes #0 = { "arg6" "arg7" }

llvm/test/Reduce/remove-call-site-attributes.ll

+; Test that llvm-reduce can remove uninteresting operand bundles from calls.
+;
+; RUN: llvm-reduce --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: cat %t | FileCheck --check-prefixes=CHECK-ALL,CHECK-FINAL %s
+
+; CHECK-ALL: declare i32 @f1(i32, i32)
+declare i32 @f1(i32, i32)
+
+; CHECK-FINAL-LABEL: define i32 @interesting(i32 %arg0, i32 %arg1) {
+; CHECK-FINAL-NEXT:  entry:
+; CHECK-FINAL-NEXT:    %r = call "attr0" i32 @f1(i32 "attr4" %arg0, i32 %arg1) #0
+; CHECK-FINAL-NEXT:    ret i32 %r
+; CHECK-FINAL-NEXT:  }
+define i32 @interesting(i32 %arg0, i32 %arg1) {
+entry:
+; CHECK-INTERESTINGNESS-LABEL: @interesting(
+
+; CHECK-INTERESTINGNESS: %r = call
+; CHECK-INTERESTINGNESS-SAME: "attr0"
+; CHECK-INTERESTINGNESS-SAME: i32 @f1(
+; CHECK-INTERESTINGNESS-SAME: i32
+; CHECK-INTERESTINGNESS-SAME: "attr4"
+; CHECK-INTERESTINGNESS-SAME: %arg0
+; CHECK-INTERESTINGNESS-SAME: i32
+; CHECK-INTERESTINGNESS-SAME: %arg1
+; CHECK-INTERESTINGNESS-SAME: #0
+; CHECK-INTERESTINGNESS: ret i32 %r
+
+  %r = call "attr0" "attr1" "attr2" i32 @f1(i32 "attr3" "attr4" "attr5" %arg0, i32 "attr6" "attr7" "attr8" %arg1) #0
+  ret i32 %r
+}
+
+; CHECK-INTERESTINGNESS: attributes #0 = {
+; CHECK-INTERESTINGNESS-SAME: "attr10"
+
+; CHECK-FINAL:  attributes #0 = { "attr10" }
+
+attributes #0 = { "attr9" "attr10" "attr11" }

llvm/test/Reduce/remove-function-attributes.ll

+; Test that llvm-reduce can remove uninteresting attributes.
+;
+; RUN: llvm-reduce --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: cat %t | FileCheck --check-prefixes=CHECK-ALL,CHECK-FINAL %s
+
+; CHECK-INTERESTINGNESS: declare
+; CHECK-INTERESTINGNESS-SAME: "attr0"
+; CHECK-INTERESTINGNESS-SAME: void @f0
+; CHECK-INTERESTINGNESS-SAME: i32
+; CHECK-INTERESTINGNESS-SAME: i32
+; CHECK-INTERESTINGNESS-SAME: "attr6"
+; CHECK-INTERESTINGNESS-SAME: #0
+
+; CHECK-FINAL: declare "attr0" void @f0(i32, i32 "attr6") #0
+
+declare "attr0" "attr1" "attr2" void @f0(i32 "attr3" "attr4" "attr5", i32 "attr6" "attr7" "attr8") #0
+
+; CHECK-INTERESTINGNESS: attributes #0 = {
+; CHECK-INTERESTINGNESS-SAME: "attr10"
+
+; CHECK-FINAL:  attributes #0 = { "attr10" }
+
+attributes #0 = { "attr9" "attr10" "attr11" }

llvm/test/Reduce/remove-global-variable-attributes.ll

+; Test that llvm-reduce can remove uninteresting attributes.
+;
+; RUN: llvm-reduce --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: cat %t | FileCheck --check-prefixes=CHECK-ALL,CHECK-FINAL %s
+
+; CHECK-ALL: @gv0 = global i32 0 #0
+; CHECK-ALL-NEXT: @gv1 = global i32 0 #1
+; CHECK-ALL-NEXT: @gv2 = global i32 0
+@gv0 = global i32 0 #0
+@gv1 = global i32 0 #1
+@gv2 = global i32 0 #2
+
+; CHECK-INTERESTINGNESS: attributes #0 = {
+; CHECK-INTERESTINGNESS-SAME: "attr0"
+; CHECK-INTERESTINGNESS-SAME: "attr2"
+
+; CHECK-INTERESTINGNESS-NEXT: attributes #1 = {
+; CHECK-INTERESTINGNESS-SAME: "attr4"
+
+; CHECK-FINAL:  attributes #0 = { "attr0" "attr2" }
+; CHECK-FINAL-NEXT:  attributes #1 = { "attr4" }
+
+; CHECK-FINAL-NOT:  attributes #2
+
+attributes #0 = { "attr0" "attr1" "attr2"}
+attributes #1 = { "attr3" "attr4" "attr5"}
+attributes #2 = { "attr6" "attr7" "attr8"}

llvm/tools/llvm-reduce/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   AllTargetsAsmParsers
   AllTargetsCodeGens
   AllTargetsDescs
   AllTargetsInfos
   Core
   IRReader
   Support
   Target
   TransformUtils
   )
 
 add_llvm_tool(llvm-reduce
   TestRunner.cpp
   deltas/Delta.cpp
   deltas/ReduceArguments.cpp
+  deltas/ReduceAttributes.cpp
   deltas/ReduceBasicBlocks.cpp
   deltas/ReduceFunctions.cpp
   deltas/ReduceGlobalVars.cpp
   deltas/ReduceInstructions.cpp
   deltas/ReduceMetadata.cpp
   deltas/ReduceOperandBundles.cpp
   llvm-reduce.cpp
 
   DEPENDS
   intrinsics_gen
   )

llvm/tools/llvm-reduce/DeltaManager.h

 //===- DeltaManager.h - Runs Delta Passes to reduce Input -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file calls each specialized Delta pass in order to reduce the input IR
 // file.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestRunner.h"
 #include "deltas/Delta.h"
 #include "deltas/ReduceArguments.h"
+#include "deltas/ReduceAttributes.h"
 #include "deltas/ReduceBasicBlocks.h"
 #include "deltas/ReduceFunctions.h"
 #include "deltas/ReduceGlobalVars.h"
 #include "deltas/ReduceInstructions.h"
 #include "deltas/ReduceMetadata.h"
 #include "deltas/ReduceOperandBundles.h"
 
 namespace llvm {
 
 // TODO: Add CLI option to run only specified Passes (for unit tests)
 inline void runDeltaPasses(TestRunner &Tester) {
   reduceFunctionsDeltaPass(Tester);
   reduceBasicBlocksDeltaPass(Tester);
   reduceGlobalsDeltaPass(Tester);
   reduceMetadataDeltaPass(Tester);
   reduceArgumentsDeltaPass(Tester);
   reduceInstructionsDeltaPass(Tester);
   reduceOperandBundesDeltaPass(Tester);
+  reduceAttributesDeltaPass(Tester);
   // TODO: Implement the remaining Delta Passes
 }
 
 } // namespace llvm

llvm/tools/llvm-reduce/deltas/ReduceAttributes.h

+//===- ReduceAttributes.h - Specialized Delta Pass ------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements a function which calls the Generic Delta pass in order
+// to reduce uninteresting attributes.
+//
+//===----------------------------------------------------------------------===//
+
+namespace llvm {
+
+class TestRunner;
+
+void reduceAttributesDeltaPass(TestRunner &Test);
+
+} // namespace llvm

llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp

+//===- ReduceAttributes.cpp - Specialized Delta Pass -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements a function which calls the Generic Delta pass in order
+// to reduce uninteresting attributes.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ReduceAttributes.h"
+#include "Delta.h"
+#include "TestRunner.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/IR/Attributes.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/InstVisitor.h"
+#include "llvm/IR/InstrTypes.h"
+#include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+#include <cassert>
+#include <iterator>
+#include <utility>
+#include <vector>
+
+namespace llvm {
+class LLVMContext;
+} // namespace llvm
+
+using namespace llvm;
+
+namespace {
+
+using AttrPtrVecTy = std::vector<const Attribute *>;
+using AttrPtrIdxVecVecTy = std::pair<unsigned, AttrPtrVecTy>;
+using AttrPtrVecVecTy = SmallVector<AttrPtrIdxVecVecTy, 3>;
+
+/// Given ChunksToKeep, produce a map of global variables/functions/calls
+/// and indexes of attributes to be preserved for each of them.
+class AttributeRemapper : public InstVisitor<AttributeRemapper> {
+  Oracle O;
+
+public:
+  DenseMap<GlobalVariable *, AttrPtrVecTy> GlobalVariablesToRefine;
+  DenseMap<Function *, AttrPtrVecVecTy> FunctionsToRefine;
+  DenseMap<CallBase *, AttrPtrVecVecTy> CallsToRefine;
+
+  explicit AttributeRemapper(ArrayRef<Chunk> ChunksToKeep) : O(ChunksToKeep) {}
+
+  void visitModule(Module &M) {
+    for (GlobalVariable &GV : M.getGlobalList())
+      visitGlobalVariable(GV);
+  }
+
+  void visitGlobalVariable(GlobalVariable &GV) {
+    // Global variables only have one attribute set.
+    const AttributeSet &AS = GV.getAttributes();
+    if (AS.hasAttributes())
+      visitAttributeSet(AS, GlobalVariablesToRefine[&GV]);
+  }
+
+  void visitFunction(Function &F) {
+    if (F.getIntrinsicID() != Intrinsic::not_intrinsic)
+      return; // We can neither add nor remove attributes from intrinsics.
+    visitAttributeList(F.getAttributes(), FunctionsToRefine[&F]);
+  }
+
+  void visitCallBase(CallBase &I) {
+    visitAttributeList(I.getAttributes(), CallsToRefine[&I]);
+  }
+
+  void visitAttributeList(const AttributeList &AL,
+                          AttrPtrVecVecTy &AttributeSetsToPreserve) {
+    assert(AttributeSetsToPreserve.empty() && "Should not be sharing vectors.");
+    AttributeSetsToPreserve.reserve(AL.getNumAttrSets());
+    for (unsigned SetIdx : seq(AL.index_begin(), AL.index_end())) {
+      AttrPtrIdxVecVecTy AttributesToPreserve;
+      AttributesToPreserve.first = SetIdx;
+      visitAttributeSet(AL.getAttributes(AttributesToPreserve.first),
+                        AttributesToPreserve.second);
+      if (!AttributesToPreserve.second.empty())
+        AttributeSetsToPreserve.emplace_back(std::move(AttributesToPreserve));
+    }
+  }
+
+  void visitAttributeSet(const AttributeSet &AS,
+                         AttrPtrVecTy &AttrsToPreserve) {
+    assert(AttrsToPreserve.empty() && "Should not be sharing vectors.");
+    AttrsToPreserve.reserve(AS.getNumAttributes());
+    for (const Attribute &A : AS)
+      if (O.shouldKeep())
+        AttrsToPreserve.emplace_back(&A);
+  }
+};
+
+struct AttributeCounter : public InstVisitor<AttributeCounter> {
+  /// How many features (in this case, attributes) did we count, total?
+  int AttributeCount = 0;
+
+  void visitModule(Module &M) {
+    for (GlobalVariable &GV : M.getGlobalList())
+      visitGlobalVariable(GV);
+  }
+
+  void visitGlobalVariable(GlobalVariable &GV) {
+    // Global variables only have one attribute set.
+    visitAttributeSet(GV.getAttributes());
+  }
+
+  void visitFunction(Function &F) {
+    if (F.getIntrinsicID() != Intrinsic::not_intrinsic)
+      return; // We can neither add nor remove attributes from intrinsics.
+    visitAttributeList(F.getAttributes());
+  }
+
+  void visitCallBase(CallBase &I) { visitAttributeList(I.getAttributes()); }
+
+  void visitAttributeList(const AttributeList &AL) {
+    for (const AttributeSet &AS : AL)
+      visitAttributeSet(AS);
+  }
+
+  void visitAttributeSet(const AttributeSet &AS) {
+    AttributeCount += AS.getNumAttributes();
+  }
+};
+
+} // namespace
+
+AttributeSet
+convertAttributeRefToAttributeSet(LLVMContext &C,
+                                  ArrayRef<const Attribute *> Attributes) {
+  AttrBuilder B;
+  for (const Attribute *A : Attributes)
+    B.addAttribute(*A);
+  return AttributeSet::get(C, B);
+}
+
+AttributeList convertAttributeRefVecToAttributeList(
+    LLVMContext &C, ArrayRef<AttrPtrIdxVecVecTy> AttributeSets) {
+  std::vector<std::pair<unsigned, AttributeSet>> SetVec;
+  SetVec.reserve(AttributeSets.size());
+
+  transform(AttributeSets, std::back_inserter(SetVec),
+            [&C](const AttrPtrIdxVecVecTy &V) {
+              return std::make_pair(
+                  V.first, convertAttributeRefToAttributeSet(C, V.second));
+            });
+
+  sort(SetVec, [](const std::pair<unsigned, AttributeSet> &LHS,
+                  const std::pair<unsigned, AttributeSet> &RHS) {
+    return LHS.first < RHS.first; // All values are unique.
+  });
+
+  return AttributeList::get(C, SetVec);
+}
+
+/// Removes out-of-chunk attributes from module.
+static void extractAttributesFromModule(std::vector<Chunk> ChunksToKeep,
+                                        Module *Program) {
+  AttributeRemapper R(ChunksToKeep);
+  R.visit(Program);
+
+  LLVMContext &C = Program->getContext();
+  for (const auto &I : R.GlobalVariablesToRefine)
+    I.first->setAttributes(convertAttributeRefToAttributeSet(C, I.second));
+  for (const auto &I : R.FunctionsToRefine)
+    I.first->setAttributes(convertAttributeRefVecToAttributeList(C, I.second));
+  for (const auto &I : R.CallsToRefine)
+    I.first->setAttributes(convertAttributeRefVecToAttributeList(C, I.second));
+}
+
+/// Counts the amount of attributes.
+static int countAttributes(Module *Program) {
+  AttributeCounter C;
+
+  // TODO: Silence index with --quiet flag
+  outs() << "----------------------------\n";
+  C.visit(Program);
+  outs() << "Number of attributes: " << C.AttributeCount << "\n";
+
+  return C.AttributeCount;
+}
+
+void llvm::reduceAttributesDeltaPass(TestRunner &Test) {
+  outs() << "*** Reducing Attributes...\n";
+  int AttributeCount = countAttributes(Test.getProgram());
+  runDeltaPass(Test, AttributeCount, extractAttributesFromModule);
+}

llvm/utils/gn/secondary/llvm/tools/llvm-reduce/BUILD.gn

 executable("llvm-reduce") {
   deps = [
     "//llvm/lib/IR",
     "//llvm/lib/IRReader",
     "//llvm/lib/Support",
     "//llvm/lib/Target",
     "//llvm/lib/Target:TargetsToBuild",
     "//llvm/lib/Transforms/Utils",
   ]
   include_dirs = [ "." ]
   sources = [
     "TestRunner.cpp",
     "deltas/Delta.cpp",
     "deltas/ReduceArguments.cpp",
+    "deltas/ReduceAttributes.cpp",
     "deltas/ReduceBasicBlocks.cpp",
     "deltas/ReduceFunctions.cpp",
     "deltas/ReduceGlobalVars.cpp",
     "deltas/ReduceInstructions.cpp",
     "deltas/ReduceMetadata.cpp",
     "deltas/ReduceOperandBundles.cpp",
     "llvm-reduce.cpp",
   ]
 }