[PATCH] D155617: Pass to annotate functions with appropriate optimization level.
Mircea Trofin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 4 08:50:11 PDT 2023
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
More information about the llvm-commits
mailing list