Can we restore review thread https://reviews.llvm.org/D125845 instead?

At the risk of abandoning more patches, I suggest reviving https://reviews.llvm.org/D124889 and update it first instead of using this one. This is to make sure all prior review history is probably kept.

LGTM (Next time, please reuse the old patch https://reviews.llvm.org/D126018 to update.)

In D125495#3516513, @kazu wrote:

In D125495#3510338, @mingmingl wrote:

In D125495#3510137, @davidxl wrote:

It might be worth annotating the early inliner for PGO as well.

To confirm if I'm understanding it correctly, you mean we should annotate the "early" (i.e., not profile-driven) information for those inline transformations that are are made without PGO profiles [1].

Ask since the early inliner is annotated with prelink/postlink information (same line around line 630 by construction of ModuleInlinerWrapperPass instance)

I am not David, but yes, it would be nice to be able to differentiate the early inlining and rhw profile-driven inlining in the FDO scenario.

This one looks fine to me. Wait to see if other reviewers have further comment.

It might be worth annotating the early inliner for PGO as well.

lgtm

Perhaps split out part of the patch that handles the following and then work on a patch to do reassociation (nikic's comment)

lgtm

Should we have a test case to cover the post-inlining scaling. Otherwise looks good.

Does the data need to be scaled first?

lgtm with nikic's comment addressed.

lgtm

lgtm

lgtm

From the feedbacks, the action items are

1. checkin baseine tests;
2. split out recurrence handling from the main patch -- have it reviewed and committed
3. send the recurrence handling patch, get it reviewed and committed (the depth may not be needed in this case)

This patch slightly changes the behavior (e.g consider alreadyPeeled when comparing with size threshold), but looks more correct. LGTM.

In D123865#3468077, @ikudrin wrote:

Unfortunatelly, I do not have access to SPEC tests. I checked with 7zip-benchmark and found that Compressing speed is slightly improved with this patch while Decompressing is slightly degraded. But I have to note that compared to a Release build, PGO one is worse even without the patch.

As for my own tests, I noticed that the performance gain with unrolled loops results from the generated code that has less instructions. In one sample, there was no need to reserve a register for the loop counters and as in each iteration the value of the counter was known, there was no instructions for increment the counter and the comparison instructions used an immediate value instead of comparing with the register; all that made the code path shorter. In another example, the subsequent iterations used overlapped data from the memory. Both compact and unrolled version loaded the data to a set of registers, but the compact version had to shift values in registers in each iteration, so that they are placed each time in the predefined registers, while unrolled version just used instructions which work this different set of registers in each iteration. Thus, unrolling again helped to find a way to generate a code with less instructions for each peeled iteration.

In D124118#3463462, @craig.topper wrote:

In D124118#3463205, @davidxl wrote:

I suspect there is something else going on.

The following case works fine without the patch.

define dso_local noundef i64 @_Z3fooPlll(ptr nocapture noundef %0, i64 noundef %1, i64 noundef %2) local_unnamed_addr #0 {

%4 = load i64, ptr %0, align 8
%5 = and i64 %4, 3
%6 = icmp eq i64 %5, 0
%7 = select i1 %6, i64 %1, i64 %2
store i64 %7, ptr %0, align 8
ret i64 %5

}

However a simple change below to replace the operand %2 in the select with %5 (output of and operation), then it stops working.

define dso_local noundef i64 @_Z3fooPlll(ptr nocapture noundef %0, i64 noundef %1, i64 noundef %2) local_unnamed_addr #0 {

%4 = load i64, ptr %0, align 8
%5 = and i64 %4, 3
%6 = icmp eq i64 %5, 0
%7 = select i1 %6, i64 %1, i64 %5
store i64 %7, ptr %0, align 8
ret i64 %5

}

Can you investigate the difference? The behavior difference on ARM is also worth comparing with.

There's a peephole in X86DAGToDAGISel::Select and PostProcessISelDAG that both fail if the AND has users other than the TEST.

I suspect there is something else going on.

A high level comment. I saw ScalingUpFactor is used in many places. It may be better to directly use Scaled64 data type instead of uint64 to avoid that.

lgtm

The patch looks reasonable. Please wait for other reviewers (Sanjay ) to comment as well.

In D123865#3454607, @ikudrin wrote:

Our test was for AArch64, so there should be no effects like loop alignment, if I understand you right.

Have you analyzed the root cause of the performance regression (with performance counters)? Is it due to other missed optimizations without the peeling or is it due to side effects such as loop alignment?

For loops with trip count between maxPeelCount and 2*maxPeelCount, this patch enables partial peeling of the loop for maxPeelCount iterations. This feels very aggressive and can be detrimental to performance: 1) increased icache footprint; 2) more pressure to BPU and 3) the back branch of the remaining loop may become less predictable.

thanks for the cleanup. LGTM

Can you add back the original list of reviewers and mark the places where the prior comments are addressed?

Is it possible to generate the covmapping file instead of checking a binary blob?

lgtm

Perhaps modify the test case to include debug statement for better coverage. Otherwise LGTM.

Should this be fixed in Clang (not generating coverage record)? Adding the logic here is a little confusing.

When the option was introduced in https://reviews.llvm.org/D120344, I think the intention is to use it to disable both sampleloader inlining (pre/postlink), so perhaps we can just tighten the comment for the option and document that the profiles are merged back.

In D121862#3389678, @hoy wrote:

In D121862#3389570, @davidxl wrote:

In D121862#3389529, @wenlei wrote:

What's your actual use case that motivated this change?

When this was originally added, we used it to disable only pre-link sample loader inlining, in which case we don't want to merge the profiles even when inlining there is skipped. This is because during post-link, sample loader inlining will still happen, so merging profiles during pre-link as if relevant inlining will never happen may cause over-optimize (it was measurable in code size). +@hoy

Good point about the post-link sample loader inlinling. The intention of this internal flag is actually to disable both sample loader inlining for performance experiment purpose (e.g exercising more cost/benefit analysis of regular inlining) -- related effort is kazu@ is doing analysis on context similarity analysis to try to tune down the sample loader inlining.

Can you shed more light on the context similarity analysis? Thanks.

In D121862#3389598, @wenlei wrote:

related effort is kazu@ is doing analysis on context similarity analysis to try to tune down the sample loader inlining.

Curious to hear more about this. Is tuning down sample loader inlining for performance or something else (e.g. compile time)? The problem of SCC inliner being bottom-up makes it less selective, which is a disadvantage comparing to early top-down inlining, so we were actually hoping to move more inlining from SCC to sample loader. Or are you shifting inlining to new ModuleInliner?

In D121862#3389529, @wenlei wrote:

What's your actual use case that motivated this change?

When this was originally added, we used it to disable only pre-link sample loader inlining, in which case we don't want to merge the profiles even when inlining there is skipped. This is because during post-link, sample loader inlining will still happen, so merging profiles during pre-link as if relevant inlining will never happen may cause over-optimize (it was measurable in code size). +@hoy

In D120230#3381568, @Amir wrote:

In D120230#3381552, @davidxl wrote:

On that internal workload, we've got 6% less cmov with this pass turned on for IRPGO (it works, no correctness issue :-) ). But perf-wise it's neutral (we can measure 0.2% perf movement on that workload with high confidence).

Does BOLT's cmov optimization improve performance for this workload?

I didn't measure it yet, but unlikely (see comment below).

Say we end up with cmov in one of the sample PGO iterations (either due to lack of profile, or profile indicating branch being unbiased), we would lose the control flow profile that is needed to tell how biased that original branch is, because we've turned that control flow into data flow. Unless we never use cmov for branches without profile info, we could keep generating cmov in future iterations even if branch becomes more biased later because we will never get control flow profile again.

If we indeed never use cmov for branch without profile, that turn this problem into a typical sample PGO oscillations. That is not the case before this patch set, are we changing the behavior now? I'm also not sure if such oscillation is as easily mitigable as other oscillations like those from speculative ICP.

Regarding BOLT's usage for this problem -- does it mean the profile data is not collected from production binary but collected using pre-BOLD binary in a training run?

Yes, the profile data should be collected from pre-BOLT binary.

If this is the setup, compiler can choose to minimze cmov generation for the sake of better profilling.

The compiler can indeed choose to minimize cmov generation – I've recently added an LLVM knob to force-expand all cmov's in D119777 (x86-cmov-converter-force-all).

However, the data collected with (non-PGO, non-LTO) clang binary suggests that x86-cmov-converter-force-all introduces a significant perf regression that BOLT's CMOV conversion with default heuristics is unable to recover from.

In D120230#3381563, @wenlei wrote:

In D120230#3381552, @davidxl wrote:

On that internal workload, we've got 6% less cmov with this pass turned on for IRPGO (it works, no correctness issue :-) ). But perf-wise it's neutral (we can measure 0.2% perf movement on that workload with high confidence).

Does BOLT's cmov optimization improve performance for this workload?

This is being worked on now and we don't have data yet. The numbers above didn't have BOLT interfering with cmov.

Say we end up with cmov in one of the sample PGO iterations (either due to lack of profile, or profile indicating branch being unbiased), we would lose the control flow profile that is needed to tell how biased that original branch is, because we've turned that control flow into data flow. Unless we never use cmov for branches without profile info, we could keep generating cmov in future iterations even if branch becomes more biased later because we will never get control flow profile again.

If we indeed never use cmov for branch without profile, that turn this problem into a typical sample PGO oscillations. That is not the case before this patch set, are we changing the behavior now? I'm also not sure if such oscillation is as easily mitigable as other oscillations like those from speculative ICP.

Regarding BOLT's usage for this problem -- does it mean the profile data is not collected from production binary but collected using pre-BOLD binary in a training run? If this is the setup, compiler can choose to minimze cmov generation for the sake of better profilling.

David

Right, making compiler conservative to preserve branch so we can have control flow profile for BOLT to make final decision is the experiment we're doing. Pseudo-probe for sample PGO can also be tuned a bit more intrusive to disallow cmov for better profile in that setup.

On that internal workload, we've got 6% less cmov with this pass turned on for IRPGO (it works, no correctness issue :-) ). But perf-wise it's neutral (we can measure 0.2% perf movement on that workload with high confidence).

FSAFDO allows late profile loading so that the matching of branches should be less of an issue.

Right. I mean pass placement.

The patch looks good to me. Adding Teresa to look at the pass ordering change.

As I mentioned, the cost of investigating performance regressions is high. However if y'all think it is better to shift the cost, I won't stay in the way.

If this is doable, it feels like a winning strategy to me.

In D120584#3362634, @nikic wrote:

There's a couple of reasons why a default-disabled option is not great:

1. It's not compatible with linker plugin LTO. People would have to be experts on LLVM internals and know that they need to add something like -Wl,-mllvm,-enable-unbounded-cross-scc-inlining=0 to their build system if they are linking rust object files.
2. This problem is not Rust-specific. According to https://discourse.llvm.org/t/rust-newpm-blocker-catastrophic-inlining/6171/2, Apple uses the earlier version of this patch, though not sure for which toolchain(s). IIRC Mozilla previously encountered this with C++ code.
3. While I agree that correctness and performance is just another tradeoff, we generally always trade off in favor of correctness, unless we expect a widespread performance impact, in which case may temporarily trade off in favor of performance. There is no evidence that this patch has any widespread impact, but there is evidence against it: For rustc performance tests, this patch is entirely performance neutral (while the previous version had minor negative impact) and the entirety of llvm-test-suite has only two programs even showing codegen changes with this patch. There might be an impact in some isolated cases (actually, this is pretty much guaranteed with any inliner change), but all evidence points towards no widespread impact, and as such I don't think there is justification for favoring performance over correctness.

It is reasonable to expect either 1) a proper/complete fix;

I do think this is a proper fix.

LLVM user base is large, the only way to get an answer to that is to turn it on and let user report -- but according to my experience, that is a very disruptive process and user may spend long time figuring out the source of regressions. From the nature of the change, the chances of that happening is quite high.

I would not object turning it on by default when the more elaborate solution is there.

Adding an option is a way to to forward, but I think it should be off by default. If it is on by default, it is very likely to cause some performance regressions.

lgtm

What is the size overhead on binary with probe? What is the impact on source drifting tolerance level?

lgtm

lgtm

Probably add a test case in file test/Transforms/SampleProfile/profile-context-tracker.ll

Is it possible to check this in the caller context?

lgtm

lgtm

lgtm

lgtm

The refactoring looks good. However in what situation do we need to modify the main header? There might be other ways to achieve the functionality.

Please add a test case. The documentation for llvm-cov tool also needs update.

lgtm