[PATCH] D110466: [llvm-profgen][CSSPGO] On-demand function size computation for preinliner

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:550-551
+                                                       uint64_t EndOffset) {
+  uint32_t Index = getIndexForOffset(StartOffset);
+  if (CodeAddrs[Index] != StartOffset)
+    WithColor::warning() << "Invalid start instruction at "
----------------
Is this the same as `addressIsCode`? or perhaps we need `addrOffsetIsCode`?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:554
+  InstructionPointer IP(this, StartAddr, true);
+  while (IP.Address < EndAddr) {
+    uint64_t Offset = virtualAddrToOffset(IP.Address);
----------------
wlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > Is the EndAddr inclusive for function end? What is we have a 1-byte instr at EndAddr?
> > EndAddr should be the start of the next function, as computed in ProfiledBinary::dissassembleSymbol. 
> Yeah, in `dissassembleSymbol ` the logic is like
> 
> ```
>   uint64_t Offset = StartOffset;
>   while (Offset < EndOffset) { 
>     ...
>     Offset += Size;
>    }
> 
> ```
> 
> so it guarantees that the end `Offset` in this function(the last `IP.Address`) won't be `EndOffset`.
> 
ok, thanks for clarification. Then the name EndOffset could be confusing. We could do StartOffset + Size (for the map in ProfiledBinary too) or rename them to avoid confusion. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110466