[PATCH] D155617: Pass to annotate functions with appropriate optimization level.

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D155617#4637329 <https://reviews.llvm.org/D155617#4637329>, @mtrofin wrote:

> lgtm, some nits you can easily address.
>
> @aeubanks, ptal - this update should address your concerns, iiuc?

yeah this is what I was imagining, thanks!

> technically you would need a test for the error case, but maybe that's overkill because this is a test-only pass.

should be pretty straightforward

  ; RUN: not opt -passes=forceattrs -func-annotator-csv-file-path=doesntexist.csv < %s | FileCheck %s --check-prefix=CSVNOTFOUND
  ; CSVNOTFOUND: Cannot open CSV file



================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:36
+static cl::opt<std::string> CSVFilePath(
+    "func-annotator-csv-file-path", cl::Hidden,
+    cl::desc("CSV file containing function names and optimization level as "
----------------
I'd shorten this to something like `forceattrs-csv-path`


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:37
+    "func-annotator-csv-file-path", cl::Hidden,
+    cl::desc("CSV file containing function names and optimization level as "
+             "attribute"));
----------------
```
Path to CSV file containing lines of function names and attributes to add to them in the form of `f1,attr1` or `f2,attr2=str`
```


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:78
 PreservedAnalyses ForceFunctionAttrsPass::run(Module &M,
-                                              ModuleAnalysisManager &) {
-  if (!hasForceAttributes())
-    return PreservedAnalyses::all();
+                                              ModuleAnalysisManager &AM) {
+
----------------
still unused, let's revert this


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:80
+
+  if (!CSVFilePath.empty()) {
+
----------------
nit: remove random empty lines


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:83
+    auto BufferOrError = MemoryBuffer::getFileOrSTDIN(CSVFilePath);
+    if (!BufferOrError) {
+      report_fatal_error("Cannot open CSV file");
----------------
nit: remove braces for one line if statements (but not ones with else statements that aren't also one line)
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:105
+          auto AttrKind = Attribute::getAttrKindFromName(SplitPair.second);
+          if (AttrKind != Attribute::None &&
+              Attribute::canUseAsFnAttr(AttrKind)) {
----------------
you can have string attributes without a value, but we can leave that for a future patch, please add a TODO


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:106
+          if (AttrKind != Attribute::None &&
+              Attribute::canUseAsFnAttr(AttrKind)) {
+            Func->addFnAttr(AttrKind);
----------------
should also report errors on this


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:113
+               << " does not exist\n";
+        continue;
+      }
----------------
should probably `report_fatal_error` if the function doesn't exist (at the end of the pass so we can report all missing functions)


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:116
+    }
+  } else {
+    if (!hasForceAttributes())
----------------
since the flags for this pass are orthogonal, we shouldn't only run the csv code if both the csv flag and the other force-function-attrs flags are passed, we should do both


================
Comment at: llvm/test/Transforms/ForcedFunctionAttrs/FunctionAnnotation.csv:1
+FunctionName,OptimizationLevel
+first_function,opt-level=O1
----------------
remove?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155617