[PATCH] D89723: [CSSPGO][llvm-profgen]Context-sensitive profile data generation

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:63
+  // Add extra frame as we unwind through the return
+  const LBREntry &LBR = State.getCurrentLBR();
+  uint64_t CallAddr = Binary->getCallAddrFromFrameAddr(LBR.Target);
----------------
wenlei wrote:
> Let `getCurrentLBR` return reference? or why would you want a temporary to bind const-reference to?
The LBR data is only kept in the UnwinderTrace and UnwindState only keep the index and ref. And because the UnwinderTrace is the key of the TraceAggregationMap, its type is converted to const. 
If that makes code confusing, I can wrap all the LBRSource/LBRTarget to getCurrentLBRSource()/getCurrentLBRTarget().


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:56
+  // Call stack recorded in anti-execution(leaf to entry) order
+  std::list<uint64_t> CallStack;
+  // LBR stack recorded in anti-execution order
----------------
wenlei wrote:
> Curious why use list for Stack but SmallVector (and Index) for LBRStack? Seem a bit inconsistent for similar use case.
Currently using list is for easy copy data from UnwinderTrace to UnwindState(see line 90 also list). Why copying the data but not using the Index like LBRStack is because Callstack is changed dynamically during the unwinding(aka need push_front)
This is kind of a temporary solution, considering our next step is to use trie node to represent callstack, at that time I will make it consistent to use SmallVector. So Ideally UnwinderTrace only keep the data and UnwindState only keep the index/trie node.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:90
+  // TODO: switch to use trie for call stack
+  std::list<uint64_t> CallStack;
+  // Used to fall through the LBR stack
----------------
wenlei wrote:
> Can this be a reference as well just like `LBRStack` and point to the input Sample? Note header comment also states "it doesn't hold the data but only keep the pointer/index of the data".
Same here, there is pop_front() with CallStack so that I used list. This will be solved by trie. The comments is the final ideal one..sorry for the confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723