llvm-reduce: Avoid invalid attribute reduction on optnone functions

[arsenm]

Summary

We have this ridiculous restriction that optnone requires noinline,
so the pair needs to be removed if we want to remove noinline.

[jdoerfert]

ridiculous restriction

+1

We should not have that. It's super annoying honestly and does not help at all. Nobody would be any worse off if we remove the assertion. Shouldn't we do that?

[arsenm]

In D140882#4023654, @jdoerfert wrote:

ridiculous restriction

+1

We should not have that. It's super annoying honestly and does not help at all. Nobody would be any worse off if we remove the assertion. Shouldn't we do that?

Yes but this is an argument I remember losing when optnone was first introduced. optnone cannot be an enforceable semantic and can act like weak noinline on its own

[jdoerfert]

In D140882#4023701, @arsenm wrote:

In D140882#4023654, @jdoerfert wrote:

ridiculous restriction

+1

We should not have that. It's super annoying honestly and does not help at all. Nobody would be any worse off if we remove the assertion. Shouldn't we do that?

Yes but this is an argument I remember losing when optnone was first introduced. optnone cannot be an enforceable semantic and can act like weak noinline on its own

I don't understand, though I vaguely remember trying to remove that assertion myself at some point.

If we don't try to do the right thing anymore, this seems to be the least wrong. LG

[arsenm]

In D140882#4023852, @jdoerfert wrote:

In D140882#4023701, @arsenm wrote:

In D140882#4023654, @jdoerfert wrote:

ridiculous restriction

+1

We should not have that. It's super annoying honestly and does not help at all. Nobody would be any worse off if we remove the assertion. Shouldn't we do that?

Yes but this is an argument I remember losing when optnone was first introduced. optnone cannot be an enforceable semantic and can act like weak noinline on its own

I don't understand, though I vaguely remember trying to remove that assertion myself at some point.

If we don't try to do the right thing anymore, this seems to be the least wrong. LG

I think this should be fixed, but I don't have the bandwidth to tilt at that particular windmill at the moment. The arguments for this was fear of QoI for some reason. It's presumptive about the implementation, since noinline actually destroys debugging on amdgpu since function debug info is incomplete (particularly upstream)

[arsenm]

7cd0fd6f519558fcccf07d7cbf14237f2c5182fe

llvm/test/tools/llvm-reduce/reduce-attributes-optnone-noinline.ll

+; RUN: llvm-reduce --abort-on-invalid-reduction --delta-passes=attributes --test FileCheck --test-arg -check-prefixes=INTERESTING,INTERESTING-NOINLINE --test-arg %s --test-arg --input-file %s -o %t.0
+; RUN: FileCheck --check-prefix=RESULT-NOINLINE %s < %t.0
+
+; RUN: llvm-reduce --abort-on-invalid-reduction --delta-passes=attributes --test FileCheck --test-arg -check-prefixes=INTERESTING,INTERESTING-OPTNONE --test-arg %s --test-arg --input-file %s -o %t.1
+; RUN: FileCheck --check-prefix=RESULT-OPTNONE %s < %t.1
+
+
+; Make sure this doesn't hit the "Attribute 'optnone' requires
+; 'noinline'!" verifier error. optnone can be dropped separately from
+; noinline, but removing noinline requires removing the pair together.
+
+
+; INTERESTING: @keep_func() [[KEEP_ATTRS:#[0-9]+]]
+; RESULT-NOINLINE: define void @keep_func() [[KEEP_ATTRS:#[0-9]+]] {
+; RESULT-OPTNONE: define void @keep_func() [[KEEP_ATTRS:#[0-9]+]] {
+define void @keep_func() #0 {
+  ret void
+}
+
+; Both should be removed together
+; INTERESTING: @drop_func()
+; RESULT-NOINLINE: define void @drop_func() {
+; RESULT-OPTNONE: define void @drop_func() {
+define void @drop_func() #0 {
+  ret void
+}
+
+; RESULT-NOINLINE: attributes [[KEEP_ATTRS]] = { noinline }
+; RESULT-OPTNONE: attributes [[KEEP_ATTRS]] = { noinline optnone }
+
+
+; INTERESTING-NOINLINE: attributes [[KEEP_ATTRS]] =
+; INTERESTING-NOINLINE-SAME: noinline
+
+; INTERESTING-OPTNONE: attributes [[KEEP_ATTRS]] =
+; INTERESTING-OPTNONE-SAME: optnone
+
+attributes #0 = { noinline optnone }

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

 namespace llvm {
 class LLVMContext;
 } // namespace llvm
 
 using namespace llvm;
 
 namespace {
 
-using AttrPtrVecTy = std::vector<const Attribute *>;
+using AttrPtrVecTy = std::vector<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;
 
for (unsigned SetIdx : AL.indexes()) {
       AttributesToPreserve.first = SetIdx;
       visitAttributeSet(AL.getAttributes(AttributesToPreserve.first),
                         AttributesToPreserve.second);
       if (!AttributesToPreserve.second.empty())
         AttributeSetsToPreserve.emplace_back(std::move(AttributesToPreserve));
     }
   }
 
+  // FIXME: Should just directly use AttrBuilder instead of going through
+  // AttrPtrVecTy
   void visitAttributeSet(const AttributeSet &AS,
                          AttrPtrVecTy &AttrsToPreserve) {
     assert(AttrsToPreserve.empty() && "Should not be sharing vectors.");
     AttrsToPreserve.reserve(AS.getNumAttributes());
-    for (const Attribute &A : AS)
+
+    // Optnone requires noinline, so removing noinline requires removing the
+    // pair.
+    Attribute NoInline = AS.getAttribute(Attribute::NoInline);
+    bool RemoveNoInline = false;
+    if (NoInline.isValid()) {
+      RemoveNoInline = !O.shouldKeep();
+      if (!RemoveNoInline)
+        AttrsToPreserve.emplace_back(NoInline);
+    }
+
+    for (Attribute A : AS) {
+      if (A.isEnumAttribute()) {
+        Attribute::AttrKind Kind = A.getKindAsEnum();
+        if (Kind == Attribute::NoInline)
+          continue;
+
+        if (RemoveNoInline && Kind == Attribute::OptimizeNone)
+          continue;
+      }
+
       if (O.shouldKeep())
-        AttrsToPreserve.emplace_back(&A);
+        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) {
struct AttributeCounter : public InstVisitor<AttributeCounter> {
 
   void visitAttributeSet(const AttributeSet &AS) {
     AttributeCount += AS.getNumAttributes();
   }
 };
 
 } // namespace
 
-AttributeSet
-convertAttributeRefToAttributeSet(LLVMContext &C,
-                                  ArrayRef<const Attribute *> Attributes) {
+AttributeSet convertAttributeRefToAttributeSet(LLVMContext &C,
+                                               ArrayRef<Attribute> Attributes) {
   AttrBuilder B(C);
-  for (const Attribute *A : Attributes)
-    B.addAttribute(*A);
+  for (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());