Force Remove Attribute

[kyulee]

Summary

-force-attribute adds an attribute to function via command-line.
However, there was no counter-part to remove an attribute.
This patch adds -force-remove-attribute that removes an attribute from function.

[kyulee]

Fix case style for ParseFunctionAndAttr

[kyulee]

Kindly ping.. This patch is convenient to chase down an issue by adding as well as removing an attribute of the function.

[plotfi]

Overall this change makes sense. I don't see why we shouldn't have this ability to remove attrs.

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

22–34

Hmm, do we care that we are changing the command line flag? Should -force-attribute imply -force-add-attribute?

31

"from a function"

81

Spell: *takes precedence.

112

Introduce hasForceAttributes as a separate NFC patch.

[plotfi]

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

22–34

@chandlerc Do you think -force-attribute should be preserved for any existing scripts or automation that may assume this?

[kyulee]

Fix typos from the feedback.

[kyulee]

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

22–34

@chandlerc @plotfi I could preserve the original name. Then I think we can have these two - force-attribute and remove-attribute.
I don't mind either direction about naming. Let me know.

112

Do you mean like this? Isn't it too simple without additional checks like this?

static bool hasForceAttributes() {
  return !ForceAttributes.empty();
}

[plotfi]

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

112

Yeah, just mention its to make this diff simpler. I'll approve it.

[lanza]

Adding jmolloy, he seems to be the original author.

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

22–34

I think the renaming isn't necessary -- it'll break any downstream users with no benefit. -force-attribute and -force-remove-attribute are a satisfactory pairing.

[kyulee]

• Preserve the original flag -force-attribute
• Factor out hasForceAttributes into D85793

[plotfi]

Please Update Diff.

[kyulee]

Rebase.

[plotfi]

I think this LGTM. @kyulee are there any other tests that use this flag? Can you add a test that both adds and also removes some different attributes. Also a test that adds and removes he same attribute too would be good.

[kyulee]

Rebase and add more tests.

[plotfi]

Ok, LGTM. I will land for you, but in your name on git. Thanks for this patch @kyulee.

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

 #include "llvm/InitializePasses.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "forceattrs"
 
-static cl::list<std::string>
-    ForceAttributes("force-attribute", cl::Hidden,
-                    cl::desc("Add an attribute to a function. This should be a "
-                             "pair of 'function-name:attribute-name', for "
-                             "example -force-attribute=foo:noinline. This "
-                             "option can be specified multiple times."));
+static cl::list<std::string> ForceAddAttributes(
+    "force-add-attribute", cl::Hidden,
+             "example -force-add-attribute=foo:noinline. This "
+             "option can be specified multiple times."));
+
+static cl::list<std::string> ForceRemoveAttributes(
+    "force-remove-attribute", cl::Hidden,
+    cl::desc("Remove an attribute from a function. This should be a "
+             "pair of 'function-name:attribute-name', for "
+             "example -force-remove-attribute=foo:noinline. This "
 
 static Attribute::AttrKind parseAttrKind(StringRef Kind) {
   return StringSwitch<Attribute::AttrKind>(Kind)
       .Case("alwaysinline", Attribute::AlwaysInline)
       .Case("builtin", Attribute::Builtin)
       .Case("cold", Attribute::Cold)
       .Case("convergent", Attribute::Convergent)
       .Case("inlinehint", Attribute::InlineHint)
return StringSwitch<Attribute::AttrKind>(Kind)
       .Case("ssp", Attribute::StackProtect)
       .Case("sspreq", Attribute::StackProtectReq)
       .Case("sspstrong", Attribute::StackProtectStrong)
       .Case("strictfp", Attribute::StrictFP)
       .Case("uwtable", Attribute::UWTable)
       .Default(Attribute::None);
 }
 
-/// If F has any forced attributes given on the command line, add them.
-static void addForcedAttributes(Function &F) {
-  for (auto &S : ForceAttributes) {
+/// If F has any forced add attributes given on the command line, add them.
+/// If F has any forced remove attributes given on the command line, remove
+/// them. When both add and remove are given to a function, the latter takes
+/// precedence.
+static void forceAttributes(Function &F) {
+  auto ParseFunctionAndAttr = [&](StringRef S) {
+    auto Kind = Attribute::None;
     auto KV = StringRef(S).split(':');
     if (KV.first != F.getName())
-      continue;
-
-    auto Kind = parseAttrKind(KV.second);
+      return Kind;
+    Kind = parseAttrKind(KV.second);
     if (Kind == Attribute::None) {
       LLVM_DEBUG(dbgs() << "ForcedAttribute: " << KV.second
                         << " unknown or not handled!\n");
-      continue;
     }
-    if (F.hasFnAttribute(Kind))
+    return Kind;
+  };
+
+  for (auto &S : ForceAddAttributes) {
+    auto Kind = ParseFunctionAndAttr(S);
+    if (Kind == Attribute::None || F.hasFnAttribute(Kind))
       continue;
     F.addFnAttr(Kind);
   }
+
+  for (auto &S : ForceRemoveAttributes) {
+    auto Kind = ParseFunctionAndAttr(S);
+    if (Kind == Attribute::None || !F.hasFnAttribute(Kind))
+      continue;
+    F.removeFnAttr(Kind);
+  }
+}
+
+static bool hasForceAttributes() {
+  return !ForceAddAttributes.empty() || !ForceRemoveAttributes.empty();
 }
 
 PreservedAnalyses ForceFunctionAttrsPass::run(Module &M,
                                               ModuleAnalysisManager &) {
-  if (ForceAttributes.empty())
+  if (!hasForceAttributes())
     return PreservedAnalyses::all();
 
   for (Function &F : M.functions())
-    addForcedAttributes(F);
+    forceAttributes(F);
 
   // Just conservatively invalidate analyses, this isn't likely to be important.
   return PreservedAnalyses::none();
 }
 
 namespace {
 struct ForceFunctionAttrsLegacyPass : public ModulePass {
   static char ID; // Pass identification, replacement for typeid
   ForceFunctionAttrsLegacyPass() : ModulePass(ID) {
     initializeForceFunctionAttrsLegacyPassPass(
         *PassRegistry::getPassRegistry());
   }
 
   bool runOnModule(Module &M) override {
-    if (ForceAttributes.empty())
+    if (!hasForceAttributes())
       return false;
 
     for (Function &F : M.functions())
-      addForcedAttributes(F);
+      forceAttributes(F);
 
     // Conservatively assume we changed something.
     return true;
   }
 };
 }
 
 char ForceFunctionAttrsLegacyPass::ID = 0;
 INITIALIZE_PASS(ForceFunctionAttrsLegacyPass, "forceattrs",
                 "Force set function attributes", false, false)
 
 Pass *llvm::createForceFunctionAttrsLegacyPass() {
   return new ForceFunctionAttrsLegacyPass();
 }

llvm/test/Transforms/ForcedFunctionAttrs/forced.ll

 ; RUN: opt < %s -S -forceattrs | FileCheck %s --check-prefix=CHECK-CONTROL
-; RUN: opt < %s -S -forceattrs -force-attribute foo:noinline | FileCheck %s --check-prefix=CHECK-FOO
-; RUN: opt < %s -S -passes=forceattrs -force-attribute foo:noinline | FileCheck %s --check-prefix=CHECK-FOO
+; RUN: opt < %s -S -forceattrs -force-add-attribute foo:noinline | FileCheck %s --check-prefix=CHECK-FOO
+; RUN: opt < %s -S -passes=forceattrs -force-add-attribute foo:noinline | FileCheck %s --check-prefix=CHECK-FOO
+; RUN: opt < %s -S -passes=forceattrs -force-remove-attribute goo:noinline | FileCheck %s --check-prefix=REMOVE
+; RUN: opt < %s -S -passes=forceattrs -force-add-attribute foo:noinline -force-remove-attribute goo:noinline | FileCheck %s --check-prefix=ADD-REMOVE
 
 ; CHECK-CONTROL: define void @foo() {
 ; CHECK-FOO: define void @foo() #0 {
 define void @foo() {
   ret void
 }
 
+; REMOVE: define void @goo() {
+; `remove` takes precedence over `add`.
+; ADD-REMOVE: define void @goo() {
+define void @goo() noinline {
+  ret void
+}
 
 ; CHECK-FOO: attributes #0 = { noinline }