[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/include/llvm/Transforms/Utils/FunctionAnnotator.h:16
+
+    #endif // LLVM_TRANSFORMS_FUNCTIONANNOTATOR_H
----------------
you need to run clang-format on your changes - (e.g. this #endif should be lined up at the beginning of the line; the `class` statement should be lined up with `namespace`

you can for example do `git clang-format HEAD~1` at this point.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:10
+PreservedAnalyses FunctionAnnotator::run(Module &M, ModuleAnalysisManager &AM) {
+  /*cl::opt<std::string> ForceAnnotate(
+      "force-annotate", cl::init(""),
----------------
remove commented code - if it's part of a future change, you can have that in your local repo (uncommented :) ).


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:34
+
+  std::ifstream CSVFile;
+  CSVFile.open(CSVFilePath);
----------------
I think we generally don't want to use `std::ifstream`. Use `MemoryBuffer`. Kind of like this:

```
auto BufferOrError = MemoryBuffer::getFileOrSTDIN(CSVFilePath);
  if (!BufferOrError) {
    Ctx.emitError("Error opening output specs file: " + CSVFilePath + " : " +
                  BufferOrError.getError().message());
    return std::nullopt;
  }
...
now use BufferOrError.get()->getBuffer()
```

Then, you can traverse the `StringRef` with `split` and so on.

The main value of the `StringRef` approach (and, IIUC, the reason it's preferred in this codebase) is it doesn't allocate lots of little string objects on the heap.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:42
+  std::string Line;
+  for (Function &F : M) {
+    while (std::getline(CSVFile, Line)) {
----------------
nit: `const Function &F`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155617