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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:404
 
+      if (MCDesc.isTerminator())
+        TerminatorOffsets.insert(Offset);
----------------
hoy wrote:
> 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? 
I see, we have the `isBranch` in MC, we can just use that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111902