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)

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

Reply via email to