lgtm, thanks.

lgtm, thanks.

Thanks for the change, and the iterator cleans up quite a bit of duplication. There's one more case left, see inline comment.

lgtm, thanks.

Thanks for the change -- good work. I can see how it improves performance of llvm-profgen. A few minor comments.

lgtm, thanks

I think we also need to decide what should always be emitted. Now there's only one warning that is always emitted. I think we can benefit from another always emit warning that can tells us when profile/binary mismatch and/or the issues like the skylake PMU bug happens. Do we get any indicator now for these issues?

The combined size of .text.hot and .text of the AutoFDO executable is 8 times as big as that of the FDO executable (even with the machine function splitting turned off).

Thanks for sharing your insights @kazu.

+@weiwang

lgtm, thanks.

lgtm except nits.

there's also an always inline pass (AlwaysInliner.cpp).

lgtm, thanks.

lgtm, thanks.

When a flat CS profile is converted to a nested profile, the call target samples for inlined callee contexts are left over in the callsite target map.

In D125255#3502490, @wlei wrote:

In D125255#3502398, @wenlei wrote:

add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.

Do we even need a threshold for emitting critical ones? If it's critical, we should always emit them?

Sorry for the confusing, " customize value for critical warning" is wrong, I meant the parameter is used to treat warnings of different importance differently. For critical warning, the threshold is 0, then it's always emitted. For non-critical ones, developer can still set a threshold or use the default one, then emit it if it's above it.

add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.

I'm not sure if a universal threshold for filtering warning is a good way to deal with this. Maybe look at this from a different angle -- for some warnings, any instance is bad, for others, we can tolerate them. Hence, an alternative: for critical ones, we should always print out any non-zero value; for others, print them only under a "verbose" mode?

In D125249#3501442, @davidxl wrote:

Does the data need to be scaled first?

thanks, lgtm assuming performance is still good with the final version.

In general, we should include code size and perf numbers for benchmarks for such changes.

lgtm, thanks.

lgtm, thanks.

Let's evaluate ignoring size limit for preinline case; otherwise lgtm, thanks.

lgtm, thanks.

Looks like this is extending https://reviews.llvm.org/D85313. Makes sense to me, but @dblaikie @jhenderson are probably better reviewers.

The description is a bit unclear. llvm-profgen giving an error isn't a reason to make this change. But if the error doesn't make sense in general, that's a reason. That's something worth explaining.

cc @weiwang

Use Optional<> for PIDFilter

lgtm, thanks.

lgtm, thanks.

Some linter seems legit.

lgtm, thanks.

Thanks for working through the changes, this looks great now. Please update the summary to reflect the latest version (we can also make it clear that this is not "on-demand" but "decode probes for profiled context only").

It is possible to let users know when this happens? Ideally it's something user need to fix - pass flags consistently.