[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:
> wlei wrote:
> > wenlei wrote:
> > > wlei wrote:
> > > > wenlei wrote:
> > > > > wlei wrote:
> > > > > > 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`.
> > > > > What I meant is we could have something like this:
> > > > > ```
> > > > >   bool addrOffsetIsCode(uint64_t Offset) const {
> > > > >     return Offset2LocStackMap.find(Offset) != Offset2LocStackMap.end();
> > > > >   }
> > > > > ```
> > > > > 
> > > > >   which is similar to addressIsCode, except that we use offset now.
> > > > > 
> > > > > 
> > > > > ```
> > > > >   bool addressIsCode(uint64_t Address) const {
> > > > >     uint64_t Offset = virtualAddrToOffset(Address);
> > > > >     return Offset2LocStackMap.find(Offset) != Offset2LocStackMap.end();
> > > > >   }
> > > > > ```
> > > > > 
> > > > > I think it's cleaner to use API instead of dealing with internals like `codeAddrOffsets` directly when possible. Also `Offset2LocStackMap` is unordered_map while `codeAddrOffsets ` is sorted vector. So look up with `Offset2LocStackMap` is faster.
> > > > I see. Here I used `uint32_t Index = getIndexForOffset(StartOffset);` because I need to use the `Index` to iterate  all the invalid instructions  which are stored in `codeAddrOffsets`. `Offset2LocStackMap ` doesn't give the `Index`.
> > > > ```
> > > > Index =  getIndexForOffset(StartOffset);
> > > > while(codeAddrOffsets[Index] <= End) {
> > > >     // use codeAddrOffsets[Index]  to do something.
> > > >     Index++;
> > > > }
> > > > ```
> > > > To avoid using `codeAddrOffsets` directly, previously we use `IP`, but IP only working on `Address` and this introduces many redundancy. i. e.  we need to change `offset` --> `address` and then change `address` back --> `offset`.
> > > > 
> > > Ok, I missed the use of that Index later.. However why do we care about invalid instructions? I assume invalid instructions won't be in the middle of a function, then for those from padding at the end, should we ignore them when computing size? 
> > Yeah, we actually only care about valid instructions here.
> > ```
> > uint32_t Index = getIndexForOffset(StartOffset);
> > uint64_t Offset = CodeAddrOffsets[Index];
> > ```
> > If `StartOffset` is an invalid offset(give a warning), it will round to next valid `Index`. for other instructions, they're all got from the `CodeAddrOffsets` by increasing the index, so they're also valid.
> I see. Though with current callers, StartOffset should always be valid offset, right? The check and warning is more about making the function robust for other use cases. 
our callers' `StartOffset` is not from `CodeAddrOffsets `.

```
uint64_t Offset = StartOffset;
while ()
    bool Disassembled = DisAsm->getInstruction...
    if (Disassembled) { 
       // CodeAddrOffsets[Offset] = ..
     }
}

FuncStartOffsetMap.emplace(StartOffset,...);
```
if the `Disassembled` for StartOffset is false, it will be an invalid start. Perhaps we can see if the warning is triggered in the future.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110466