wlei-llvm wrote:
> > > > > > Ping @wlei-llvm
> > > > >
> > > > >
> > > > > Sorry for the delay. The new version addressed my last comment (with
> > > > > just minor nits). However, I didn't fully follow the new features
> > > > > related to `ProbeMatchSpecs` stuffs. Could you add more descriptions
> > > > > to the diff summary? Or if it’s not a lot of work, could we split it
> > > > > into two patches? We could commit the first part, and I will review
> > > > > the second part separately.
> > > >
> > > >
> > > > NVM, I think now I get what `ProbeMatchSpecs` does, it's a vector
> > > > because a function can have multiple sections(function split)
> > >
> > >
> > > Thank you for reviewing and sorry for the delay from my end, was busy
> > > with profile quality work.
> > > ProbeMatchSpecs is a mechanism to match probes belonging to another
> > > binary function. I'm going to utilize it in probe-based function matching
> > > (#100446). For example:
> > > source function:
> > > ```
> > > void foo() {
> > > bar();
> > > }
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > profiled binary: bar is not inlined => have top-level function bar new
> > > binary (where the profile is applied to): bar is inlined into foo.
> > > Right now, BOLT does 1:1 matching between profile functions and binary
> > > functions based on the name. #100446 will extend this to N:M where
> > > multiple profiles can be matched to one binary function (as in the
> > > example above where binary function foo would use profiles for foo and
> > > bar), and one profile can be matched to multiple binary functions (eg if
> > > bar was inlined into multiple functions).
> > > In this diff, ProbeMatchSpecs would only have one BinaryFunctionProfile
> > > (existing name-based matching).
> >
> >
> > Thanks for the explanation! I was confused of the use of `ProbeMatchSpecs`,
> > it would be great to add more descriptions in the diff summary or somewhere
> > in the comments, or in the another patch when it's used(IMO if we add a
> > feature, but we doesn't use it in the patch, it should be in the future
> > patch when it's used)
>
> Thank you. Added description of ProbeMatchSpecs to the summary. I decided to
> introduce it in this diff because there's tight coupling between probe-based
> block matching and function matching. This way, probe-based function matching
> would not need to change how block matching works.
Makes sense, thanks for clarifying!
https://github.com/llvm/llvm-project/pull/99891
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
