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

[Lei Wang via Phabricator via llvm-commits]

wlei 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 "
----------------
wenlei wrote:
> Is this the same as `addressIsCode`? or perhaps we need `addrOffsetIsCode`?
The input of `addressIsCode` is the `address`. I guess the confusing part is `CodeAddrs` so I changed to `codeAddrOffsets`.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:554
+  InstructionPointer IP(this, StartAddr, true);
+  while (IP.Address < EndAddr) {
+    uint64_t Offset = virtualAddrToOffset(IP.Address);
----------------
wenlei wrote:
> 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. 
Yeah, I changed some code in `dissassembleSymbol ` to ensure the `EndOffset` is the last valid offset of the function and the previous EndOffset is renamed to `NextStartOffset`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110466