[PATCH] D110864: [llvm-profgen] Ignore branch count against outline function

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:404
+    return false;
+  return CalleeName.find(".") != StringRef::npos;
+}
----------------
wlei wrote:
> hoy wrote:
> > wlei wrote:
> > > wlei wrote:
> > > > hoy wrote:
> > > > > Check against specific suffix here so that we don't overlook cases we are not aware of?
> > > > So I guess you mean we can't assume here it covers all the outlined function case, right? I was thinking all the non-outlined cases are all handled by `getCanonicalFnName`.
> > > If we have an outlined function here, say it's name is `foo.cold`, it's function body code is still count against `foo` because we use the name from debug table. so all the body samples are collected into `foo` 's FunctionSamples. This is the right way.
> > > 
> > > But for the head samples, here we used the name from symbol table, so if we count this, we will have a sample line like 
> > > ```
> > > foo.cold:0:100
> > > ```
> > > means the total_samples is zero and head samples is not.
> > > 
> > > So I'm wondering if we don't do anything for `populateBoundarySamples` and we have a post-process to trim all the FunctionSamples that headsamples is non-zero but total_samples is zero.
> > > 
> > > With this, I guess we won't overlook any cases. what do you think?
> > > 
> > > 
> > Actually I meant the change here is a bit too general. We are assuming names with a suffix other than .__uniq, are all for backend-outlined functions. I'm not quite sure that's the case so suggesting we just check against .resume here. For example, the front end could outline functions and when it comes to sample loader, those outlined functions should have their own profiles.
> That makes sense, thanks!
> 
> Then how about we just leave it unchanged like before(we can leave the test in this patch). 
> 
> Currently we already use the name from symbol table, so we won't have the issue as our internal tool which used from debug table.
> 
> The only `issue` is we might have some sample like `foo.cold:0:100`, but it won't have impact.
> 
> If it's a pre-SampleLoader outlined function, compiler can consume it otherwise it just ignore it.
> 
> 
> 
> 
> 
> 
> 
That sounds good too. The extra profile there will just be ignored by the sample loader.


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

https://reviews.llvm.org/D110864