[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:32
+static cl::opt<bool>
+    IsHeaderRow("is-header-row", cl::init(true), cl::Hidden,
+                cl::desc("True if the first row of the CSV file is a header "
----------------
Nit: I'd avoid having this as a flag. Let's just say the first row must be header.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:55
+        if (Func->isDeclaration()) {
+          ++It;
+          continue;
----------------
how about instead of a while loop:

`for(; !It.is_at_end(); ++It) {`

then no need for `++It;` everywhere


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:60
+      } else {
+        outs() << "Function in CSV file at line " << It.line_number()
+               << " does not exist\n";
----------------
I think you want to use `errs()` here. Probably could use LLVMContext::diagnose, too, but since this is for test, `errs()` should be fine. In any case, not `outs()`.


================
Comment at: llvm/test/Transforms/Annotations/FunctionAnnotator.ll:2
+; RUN: opt -passes='module(function-annotator)' -func-annotator-csv-file-path="%S/FunctionAnnotation.csv" -S < %s | FileCheck %s
+; CHECK-LABEL: @first_function
+define void @first_function() {
----------------
you want to check also that first_function has associated with it O1, while second_function has O3... etc. If you run your opt command line over this .ll file, you should see how the annotations appear in the output text.

also check stderr for the messages saying that fourth and fifth functions weren't found.

For the latter, just add `2>&1` to the end of the `opt` command line, so you can combine stdout and stderr together to what FileCheck sees. the error messages for fourth and fifth functions should come in order - because of how you traverse the csv.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155617