[Mlir-commits] [clang] [compiler-rt] [libc] [lld] [lldb] [llvm] [mlir] [BOLT] Match blocks with pseudo probes (PR #99891)

[Amir Ayupov]

aaupov 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.

https://github.com/llvm/llvm-project/pull/99891