+ @xur

Google keeps zero counts in the profile. It treats missing lines conservatively and tries to infer their hotness. I vaguely remember there is a flag added to control it.

In D104129#2821189, @hoy wrote:

In D104129#2821164, @wenlei wrote:

Be consistent with dwarf line number based profile.

This is not accurate, see earlier comments. What i meant was not about "non-probe profile -> dwarf line number profile".

Otherwise lgtm. Thanks.

It should be consistent with non-CS dwarf-based profile. CS dwarf-based profile doesn't have either zero or dangling reported, likely needs a fix. Is that what you meant?

Be consistent with dwarf line number based profile.

In D104129#2819438, @wenlei wrote:

Be consistent with non-probe profile.

AutoFDO profile usually does not fill in zeros. counts below a certain threshold is omitted to save profile size.

Now in order to differentiate from unknown, the approach taken here is to always mark any known count including zero. In that sense this is different from AutoFDO (actually before this change, dangling probes are marked so zero counts can be omitted which is closer to AutoFDO for representing sparse profile).

Improving counts quality by respecting the counts already collected on the non-dangling sibling of the danling probe

nit on the wording, sibling leads others to think it's a different probe under the same probe inline tree. Here, it's really just copies of the same probe (probes sharing the same Id).

This is blocking our internal adoption of llvm-12, going to commit on behalf of Igor now.

Makes sense to me to still allow overriding. Thanks for the change.

Be consistent with non-probe profile.

Thanks for working on this. Among all deleted probes, if the majority of them are actually dangling (i.e removed due to hoisting, merging like optimization as opposed to dce), it makes sense to default to dangling and explicitly marking non-dangling. Previously we assumed that majority of deleted probes are not dangled, hence we choose to mark dangled ones explicitly.

lgtm, thanks.

In D104099#2814167, @davidxl wrote:

Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads.

lgtm, thanks!

lgtm, thanks for working on this Sergey!

In D103867#2814684, @lxfind wrote:

Poking around it looks like this should be handled for inlining, not sure why the previous case we ran into wasn't covered by that code below from CallAnalyzer::analyze()

// Disallow inlining a blockaddress with uses other than strictly callbr.
// A blockaddress only has defined behavior for an indirect branch in the
// same function, and we do not currently support inlining indirect
// branches.  But, the inliner may not see an indirect branch that ends up
// being dead code at a particular call site. If the blockaddress escapes
// the function, e.g., via a global variable, inlining may lead to an
// invalid cross-function reference.
// FIXME: pr/39560: continue relaxing this overt restriction.
if (BB->hasAddressTaken())
  for (User *U : BlockAddress::get(&*BB)->users())
    if (!isa<CallBrInst>(*U))
      return InlineResult::failure("blockaddress used outside of callbr");

LGTM, thanks for the fix.

I think because that code is only checking for CallBrInst users. But in a computed goto pattern, the addresses are usually just stored in a global variable.

In D103867#2814096, @hoy wrote:

In D103867#2806612, @aeubanks wrote:

do all passes that clone BBs have to worry about this?

I think so. We've seen a similar case in inlining where an address-taken block was inlined. The inlining should be disabled as well.

Thanks for the patch. I think this can be useful for tuning. However as we discussed, it'd be nice if the level of merging (number of frames to keep after merging) can be adaptive based on hotness. But to do that, the preinliner in llvm-profgen is the way to go.

cc @davidxl @xur @wmi

lgtm, thanks for the fix.

We also saw ipa-cp-clone being a very noticeable difference between gcc and llvm when we tried to move workloads from gcc to llvm. Thanks for working on this for llvm optimization parity with gcc.

cc @wmi @xur @davidxl

lgtm, thanks.

lgtm, thanks.

Thanks for the change. I tried a few limits on the repro case I mentioned in D91722. It does look like 16 is a sweet spot at least for that case. :) Without forcing a limit, the case runs for 10+ hours..

In D91722#2781418, @StephenTozer wrote:

In D91722#2781128, @wenlei wrote:

We're seeing a pathological case with 36ec97f76ac0d8be76fb16ac521f55126766267d (3rd Reapply "[DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands).

I didn't encounter any such cases while testing this patch, but it's certainly possible for it to occur. Personally, I think the best approach for this is to impose a limit on the number of args that can be added to a DIArgList by salvaging - it's clear that it is not feasible to salvage everywhere we possibly can. I think that a max of 32 is probably a reasonable amount, but if you have a reproducer for this issue I can do some performance testing myself to more accurately determine a sensible limit.

I do believe that InstCombine also has a problem with massively inflating the number of debug intrinsics in a function; the slowdown factor you're seeing is probably the product of these two factors.

We're seeing a pathological case with 36ec97f76ac0d8be76fb16ac521f55126766267d (3rd Reapply "[DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands).

lgtm with a nit, thanks for the fix.

lgtm, thanks.

lgtm, thanks.

lgtm too, thanks. looking forward to complete fs-adfo support and we'd be happy to give it a try.

In D102537#2762431, @hoy wrote:

In D102537#2761666, @wenlei wrote:

Thanks for bringing this up - it makes sure we give full context profile precedence even when we had to use context-less profile in prelink.

When counts are accurate, I was seeing #1 and #2 help reduce code size by preventing post-sample ICP and CGSCC inliner working on obselete metedata.

I assume the above is from CSSPGO, right?

It'd be good to call out in the change description as to why postlink overwrite could be beneficial.

Yes, they are for CSSPGO.

If we target AutoFDO as well, would be good to mention the impact on AutoFDO too.

I haven't tested for non-CS case. In theory it should help there but with a less-than-ideal counts quality, the change may not help in practice.

Thanks for bringing this up - it makes sure we give full context profile precedence even when we had to use context-less profile in prelink.

lgtm, with some nit on comments.

In D102246#2760712, @xur wrote:

Integrated Wenlei's suggestions.

@wenlei: I only moved encoding / decoding static functions from DebugInfoMetadata.h to the new Support/Discriminator.h and left other discriminator codes in DebugInfoMetadata.h. Not sure if this is what you meant in the comments.

lgtm, thanks.

cc: @wlei

good catch, thanks for the fix!

lgtm, thanks for the fix.

So without this fix, it would AV as well if a block only had dbg instruction?

lgtm

lgtm, thanks for addressing the ICP issues.

lgtm, thanks.

Ignore trim/merge flag for non-CS profile.

The dependency on Analysis should be removed in the latest version.

Cleanup.

Move threshold knobs into ProfileSummaryBuilder, avoid pulling Analysis lib into tools.

lgtm, thanks!

lgtm, thanks.

Thanks for the fix. How about using DAG check for things under other "Getting base profile for function" too?

In D100235#2683507, @hoy wrote:

In D100235#2683311, @wenlei wrote:

In D100235#2682347, @hoy wrote:

In D100235#2681312, @wenlei wrote:

Another issue being fixed is that all outlined functions shared the same inline frame previously due to the unchanged Index value as the dummy inlineSite identifier.

Why is this an issue? What problem does this cause? Conceptually, if we want to give them a tree structure, top level inliner are indeed children of dummy root.

Yes, top level inliner is still children of dummy root with this change. However, they do not share dummy callsite id any more. Note that without this change, the dummy callsite id for top-level inliner (Index below) isn't updated correctly.

if (Root != Cur) {
   Index = readUnsignedNumber<uint32_t>();
 }

Yeah, index would always be zero. But what problem does this cause? It seems that the call site id for top-level inliner is a dummy field anyways, so value should be ignored. Currently it works just fine, right? Is it also motivated by the upcoming change to report dangling probes with counts (so you need to different call site under dummy root)? If so, it might make sense to include it there as well..

The index starts from zero, and could be updated by parsing an inlinee. And when it comes back to the next top-level inliner, the value index is the leftover from the previous inlinee. This causes top-level inliner frames non-deterministically shared. The fix is motivated by upcoming dangling probe work but I feel that it may also affect inliner context building for shared top-level frames. I'll see if I can find a case for that. Otherwise I'll include this fix in the dangling probe patch as well.

In D100235#2682347, @hoy wrote:

In D100235#2681312, @wenlei wrote:

Another issue being fixed is that all outlined functions shared the same inline frame previously due to the unchanged Index value as the dummy inlineSite identifier.

Why is this an issue? What problem does this cause? Conceptually, if we want to give them a tree structure, top level inliner are indeed children of dummy root.

Yes, top level inliner is still children of dummy root with this change. However, they do not share dummy callsite id any more. Note that without this change, the dummy callsite id for top-level inliner (Index below) isn't updated correctly.

if (Root != Cur) {
   Index = readUnsignedNumber<uint32_t>();
 }