[PATCH] D111902: [llvm-profgen] Warn on invalid range and show warning summary

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:404
 
+      if (MCDesc.isTerminator())
+        TerminatorOffsets.insert(Offset);
----------------
wlei wrote:
> hoy wrote:
> > wlei wrote:
> > > hoy wrote:
> > > > Is this supposed to identify a branch instruction? `BranchAddrs` may sound easier to understand?
> > > isTerminator can be branches and return, refer to the comments from `isTerminator`
> > > ```
> > >   /// Returns true if this instruction part of the terminator for
> > >   /// a basic block.  Typically this is things like return and branch
> > >   /// instructions.
> > >   ///
> > > ```
> > > 
> > > Added comments on it.
> > I see. Return instructions can also be considered branch instructions in the profile generator. They are basically jumps. We use `TerminatorOffsets` to check if the last instruction of a range is a jump, right? Terminator sounds a bit formal. Branches or jumps may be more natural to understand. What do you think? 
> Yeah, this term is a little bit ambiguous.  I have the function to check if this is a jump, see below.
> ```
>  bool offsetIsBoundry(uint64_t Offset) {
>     return TerminatorOffsets.count(Offset) || CallAddrs.count(Offset);
>   }
> ```
> So `Terminator` is `branch` or `return` but not `call`. So we should check both `Terminato`r and `call` here. jump seems including `call`.
> 
> Maybe rename it to `BranchOrReturnOffsets`?
> 
> 
> 
I see. We already have CallAddrs and RetAddrs, maybe make a BranchAddrs for isTerminator && !isReturn? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111902