[PATCH] D85586: Force Function Attributes

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee marked 2 inline comments as done.
kyulee added inline comments.


================
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,
----------------
plotfi wrote:
> plotfi wrote:
> > Hmm, do we care that we are changing the command line flag? Should -force-attribute imply -force-add-attribute? 
> @chandlerc Do you think -force-attribute  should be preserved for any existing scripts or automation that may assume this?
@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.


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:112
+
+static bool hasForceAttributes() {
+  return !ForceAddAttributes.empty() || !ForceRemoveAttributes.empty();
----------------
plotfi wrote:
> Introduce `hasForceAttributes` as a separate NFC patch. 
Do you mean like this? Isn't it too simple without additional checks like this?
```
static bool hasForceAttributes() {
  return !ForceAttributes.empty();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85586