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

[Puneeth via Phabricator via llvm-commits]

Puneeth-A-R marked 10 inline comments as done.
Puneeth-A-R 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,
----------------
mtrofin wrote:
> 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.
Will be talking about the test case on call.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:20
+
+  auto BufferOrError = MemoryBuffer::getFileOrSTDIN(CSVFilePath);
+  if (!BufferOrError) {
----------------
mtrofin wrote:
> 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.
Oh yeah! Silly me.


================
Comment at: llvm/lib/Transforms/Utils/FunctionAnnotator.cpp:26
+  StringRef Buffer = BufferOrError.get()->getBuffer();
+  auto memoryBuffer = MemoryBuffer::getMemBuffer(Buffer);
+  line_iterator itr(*memoryBuffer, false);
----------------
mtrofin wrote:
> naming - first letter capital
> 
> but more importantly, doesn't BufferOrError.get() give you the memory buffer?
BufferOrError.get() seems to return llvm::ErrorOr<std::__1::unique_ptr<llvm::MemoryBuffer>> and not a plain 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) {
----------------
mtrofin wrote:
> 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"
I wonder why... just a convention or is there a specific reason?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155617