[PATCH] D85586: Force Function Attributes

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added a comment.

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



================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:22
 
-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,
----------------
Hmm, do we care that we are changing the command line flag? Should -force-attribute imply -force-add-attribute? 


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:31
+    "force-remove-attribute", cl::Hidden,
+    cl::desc("Remove an attribute to a function. This should be a "
+             "pair of 'function-name:attribute-name', for "
----------------
"from a function"


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:81
+/// 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 taks
+/// precedence.
----------------
Spell: *takes precedence.


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:112
+
+static bool hasForceAttributes() {
+  return !ForceAddAttributes.empty() || !ForceRemoveAttributes.empty();
----------------
Introduce `hasForceAttributes` as a separate NFC patch. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85586/new/

https://reviews.llvm.org/D85586