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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

lgtm, some nits you can easily address.

@aeubanks, ptal - this update should address your concerns, iiuc?

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



================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:35
 
+static cl::opt<std::string>
+    OptLevelAttributeName("opt-level-attribute-name", cl::Hidden,
----------------
you don't need this anymore, since it's taken care of by the csv format.


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:107
+          } else {
+            auto attrkind = Attribute::None;
+            attrkind = Attribute::getAttrKindFromName(SplitPair.second);
----------------
Attrkind (style guide)

also, since you're setting it right after from the `getAttrKindFromName` function call, just initialize it from that rather than setting it to `None`, then calling that function.


================
Comment at: llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp:115
+        } else {
+          errs() << "Function in CSV file at line " << It.line_number()
+                 << " does not exist\n";
----------------
Nit: it is more readable, I think, if you did this:

```
for(++It... etc)
  auto SplitPair = ..
  if (SplitPair.second.empty()) {
    errs() << ...
    break;
  }
  ...
```

because you don't need an `else` (since you break), and thus less indentation. 

Also `break` because... why bother continuing. It's an error anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155617