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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:11
+
+  static cl::opt<std::string> OptLevelAttributeName(
+      "-opt-level-attribute-name", cl::init(""), cl::Hidden,
----------------
move the flags outside the function, right after line 7 - see for instance https://github.com/llvm/llvm-project/blob/d6f1880c629d629d03914ad564b4d7b188ada444/llvm/lib/Transforms/IPO/Inliner.cpp#L75


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:11
+
+  static cl::opt<std::string> OptLevelAttributeName(
+      "-opt-level-attribute-name", cl::init(""), cl::Hidden,
----------------
mtrofin wrote:
> move the flags outside the function, right after line 7 - see for instance https://github.com/llvm/llvm-project/blob/d6f1880c629d629d03914ad564b4d7b188ada444/llvm/lib/Transforms/IPO/Inliner.cpp#L75
I'm a bit confused how the test works since it's not passing `-opt-level-attribute-name`?

That aside, maybe have the default value not be `""` but instead the actual attribute name. If there can only be one value, maybe just make this a const char* for now.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:16
+  static cl::opt<std::string> CSVFilePath(
+      "-csv-file-path", cl::Hidden, cl::Required,
+      cl::desc("CSV file containing function names and optimization level as "
----------------
could you give this flag a less general name like `-func-annotator-csv-file-path`


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:20
+
+  auto BufferOrError = MemoryBuffer::getFileOrSTDIN(CSVFilePath);
+  if (!BufferOrError) {
----------------
Puneeth-A-R wrote:
> @mtrofin Does this need to be closed? If so, how to go about doing it? From several examples that I saw, it seems this doesn't need to be closed.
See the implementation - the file is loaded in entirety in the memory buffer, then closed.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:26
+  StringRef Buffer = BufferOrError.get()->getBuffer();
+  auto memoryBuffer = MemoryBuffer::getMemBuffer(Buffer);
+  line_iterator itr(*memoryBuffer, false);
----------------
naming - first letter capital

but more importantly, doesn't BufferOrError.get() give you the memory buffer?


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:27
+  auto memoryBuffer = MemoryBuffer::getMemBuffer(Buffer);
+  line_iterator itr(*memoryBuffer, false);
+  for (Function &F : M) {
----------------
nit: why SkipBlanks=false? (fine by me, the csv is for test - just curious) 


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:27
+  auto memoryBuffer = MemoryBuffer::getMemBuffer(Buffer);
+  line_iterator itr(*memoryBuffer, false);
+  for (Function &F : M) {
----------------
mtrofin wrote:
> nit: why SkipBlanks=false? (fine by me, the csv is for test - just curious) 
Itr (first letter must be capital)

we tend to call iterators "I" or "It"


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:33
+
+    while (!itr.is_at_end()) {
+      if (itr.operator*().split(',').second != "") {
----------------
This traversal assumes the order in which you'll visit the functions in the module coincides with the order they are in the csv. While this is for test, it's also fragile and easily avoidable: you can iterate through the csv, and use `Module::getFunction(StringRef)` to fetch the corresponding `Function*` - which will be `nullptr` if it's not defined/declared.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:34
+    while (!itr.is_at_end()) {
+      if (itr.operator*().split(',').second != "") {
+        if (itr.operator*().split(',').first == F.getName()) {
----------------
I think the preferred style is to use "empty()", i.e. `if (!It->split(',').second.empty())`

also - while at it. Don't call `operator*()`, just dereference, so `It->split()` should work.
(this last comment applies in a few more places)


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:46
+}
\ No newline at end of file

----------------
newline


================
Comment at: llvm/test/Transforms/Annotations/FunctionAnnotation.csv:7
+fifth_function,O3
\ No newline at end of file

----------------
newline


================
Comment at: llvm/test/Transforms/Annotations/FunctionAnnotator.ll:1
+; RUN: opt -passes='module(function-annotator)' -csv-file-path="./FunctionAnnotation.csv" < %s | FileCheck %s
+
----------------
more canonical is to have `-csv-file-path=%S/FunctionAnnotation.csv`

`%S` will be substituted to the directory of the .ll you're running.



================
Comment at: llvm/test/Transforms/Annotations/FunctionAnnotator.ll:3
+
+; CHECK_LABEL: AttributeList
+; CHECK-NEXT: AttributeList[
----------------
this .ll should define some functions - specifically, those you name in the csv. can be as easy as

```
define void @first_function() {
  ret
}
```

etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155617