Document the new llvm-profdata option in the Commandline guide.

looks good to me (and less confusing ;) ).

In D145008#4162517, @nikic wrote:

In D145008#4162106, @davidxl wrote:

In D145008#4160793, @nikic wrote:

The value may be poison even if it's not literally poison. The problem seems to be that the condition gets swapped. CHR generates select i1 poison, i1 %arg, i1 false rather than the correct select i1 %arg, i1 poison, i1 false. Either that swapping should not occur (if it's not important to what CHR is doing) or the condition needs to be frozen.

Correct -- kazu@ brought it up before sending the patch as well -- is there existing interface to query for propagated poison?

There is isGuaranteedNotToBeUndefOrPoison(), but here you want to use freeze as a fallback.

The swap is triggered because of the profile data says it is profitable -- while it is itself a bug to deal with, it is independent to this fix.

How can the swap be profitable when flattening control flow into a select? Shouldn't it be independent of order at that point?

In D145008#4160793, @nikic wrote:

The value may be poison even if it's not literally poison. The problem seems to be that the condition gets swapped. CHR generates select i1 poison, i1 %arg, i1 false rather than the correct select i1 %arg, i1 poison, i1 false. Either that swapping should not occur (if it's not important to what CHR is doing) or the condition needs to be frozen.

lgtm

There is a related patch by Arthur: https://reviews.llvm.org/D143424. Can that patch solve the problem?

In D98213#4070466, @aemerson wrote:

Note the size here is not static code size (it excludes cold code). It is actually a proxy to model the runtime cost due to increased instruction footprint (icache pressure). The multiplier's role is to make the savings and 'size' cost comparable in terms of unit. The cycle name here is also counted at IR level, so not at low level.

I understand that, but it still doesn't make the comparison of different units any better. Introducing a scaling factor is irrelevant, it's just an arbitrary scalar.

In D98213#4070344, @aemerson wrote:

In D98213#4070282, @davidxl wrote:

In D98213#4070227, @aemerson wrote:

I want to resurrect this change that enabled these cost-benefit heuristics originally developed in https://reviews.llvm.org/D92780

There are a number of issues with the original change that I can see, and this has caused all manner of problems for us internally as we moved to the new pass manager (and therefore started using these heuristics).

The main one being that the heuristics seem to completely override all sensible thresholds for code size. The feature caims to compare the performance gains by measuring the cycles saved from a PGO run, and comparing against some threshold. This doesn't make sense since the length of the runs for PGO are arbitrary, and you shouldn't be using them in absolute terms. The only way it makes sense is if you have a very specific workload and the PGO profile run is exactly the same as the real world runs. The original change gave absolutely no justification, formal or intuitive, for the use of the equation to determine profitability. Giving positive runtime results is no justification for completely overriding any reasonable thresholds and potentially impacting code size & compile time by several orders of magnitude as we've seen internally at Apple.

Even worse: D92780 was accepted into mainline LLVM with absolutely no tests, and then enabled for all users in this change. I didn't even see any discussion about tests, or explanation for why they were impractical. We've disabled these heuristics downstream but I absolutely believe that this must be reverted upstream too until it can be re-worked and re-reviewed in detail.

FYI: the cycle savings are normalized by dividing the function entry count, so it is not used in absolute sense.

Right, that's fair enough. See the following comment from the code:

//  CycleSavings      PSI->getOrCompHotCountThreshold()
// -------------- >= -----------------------------------
//       Size              InlineSavingsMultiplier

Cycles are being compared against code size here. How is that metric supposed to be interpreted? What does (cycles/entry)/(bytes) even mean as a unit of measurement? The feature itself seems to be trying to optimize too fine details in machine cycles in a part of the compiler that shouldn't be dealing with such low level specifics.

In D98213#4070227, @aemerson wrote:

I want to resurrect this change that enabled these cost-benefit heuristics originally developed in https://reviews.llvm.org/D92780

There are a number of issues with the original change that I can see, and this has caused all manner of problems for us internally as we moved to the new pass manager (and therefore started using these heuristics).

The main one being that the heuristics seem to completely override all sensible thresholds for code size. The feature caims to compare the performance gains by measuring the cycles saved from a PGO run, and comparing against some threshold. This doesn't make sense since the length of the runs for PGO are arbitrary, and you shouldn't be using them in absolute terms. The only way it makes sense is if you have a very specific workload and the PGO profile run is exactly the same as the real world runs. The original change gave absolutely no justification, formal or intuitive, for the use of the equation to determine profitability. Giving positive runtime results is no justification for completely overriding any reasonable thresholds and potentially impacting code size & compile time by several orders of magnitude as we've seen internally at Apple.

Even worse: D92780 was accepted into mainline LLVM with absolutely no tests, and then enabled for all users in this change. I didn't even see any discussion about tests, or explanation for why they were impractical. We've disabled these heuristics downstream but I absolutely believe that this must be reverted upstream too until it can be re-worked and re-reviewed in detail.

lgtm

great cleanup overall.

The check looks unnecessary or misplaced.

Is the MIB removal purely for the purpose of reducing memory usage? It seems safe to always keep them.

It might be better to ask for commit access. It is needed for potential follow-ups.

lgtm

never mind. I misunderstood the usage of this variable. There is no need to adjust.

It seems that the explosion can happen when the always-inline callee has multiple call edges within the nontrivial SCC. Should the check be more refined?

Add a test case for completeness.

lgtm

lgtm

thanks for reverting. There is test case available to identify the root cause of regressions.

This looks good. Sotiris may help with a quick benchmarking for the patch.

lgtm

lgtm

I see. The DOT CFG graph dump uses the layout order instead of the MBB number. Another way to fix this is to use the following in the node label: BBName[MBBNum][layout-order], but making the MBB num and layout number consistent under an option is fine to me.

or show an example how the mapping is made harder without renumbering.

Does MIR text output show MBB number in the dump?

should be

y  = tmp + i + x;

This is essentially forward propagation (with multi-use) and is a generally a good way to reduce dependence height. For instance it can be used to enable reassociation and LICM:

lgtm

Add a small test case (with small threshold)?

In D135929#3886921, @gulfem wrote:

In D135929#3886182, @davidxl wrote:

What is the use case for the feature?

Our current coverage pipeline has the following steps:

1. Read binary-id in each raw profile (.profraw) via using llvm-profdata
2. Fetch all the binaries with the corresponding binary id
3. Merge all the raw profiles, and generate an indexed profile (.profdata)
4. Generate a coverage report via using llvm-cov by providing all the fetched binaries and indexed profile.

We would like to simplify our coverage pipeline by using debuginfod in llvm-cov. When we include binary ids into indexed profiles and start using debuginfod in llvm-cov, our coverage pipeline can look like this:

1. Merge all the raw profiles, and generate an indexed profile (.profdata)
2. Generate a coverage report via using llvm-cov by providing only an indexed profile. llvm-cov is going to read all the binary ids from an indexed profile, and fetch the associated binaries from the debuginfod server.

What is the use case for the feature?

For incremental build, are the artifacts (build logs) also cached?

The use case of the feature is still a little fuzzy to me. Why is the mismatch issue not caught and handled at build time?

lgtm

Naive question -- can this be done as a post-processing step with proper reordering?

Vasileios, Abataev's suggestion seems reasonable. Can you try it?

In D129734#3853403, @huangjd wrote:

follow up @davidxl
For the original issue where a chain of 3 or more gep with the first and last being constant indexed cannot be simplified, this patch can handle such case while D125845 can't
Consider the following code

01   b = gep a, const_index
02   use(b)
03   c = gep b, var_index
04   d = gep c, const_index
05   ret d

In this patch line 4 is swapped with line 3, and then it is constant-index folded with line 1 (line 1 is unchanged, and the old line 4 is replaced with gep a with new constant indices, breaking the dependency of b, and result in better codegen.

But if D125845 is used, then line 1 is supposed to be swapped with line 3, but since b has more than 1 use, the swap is inhibited, and nothing can be optimized.

sounds fine to me.

Is it due to the mixing instrumented shared libraries built with different compiler and profile versions?

lgtm

1. the CommandLine documentation file needs to be updated.
2. what is the use case of splitting text and text-encoding?

In D134152#3821184, @fhahn wrote:

In D134152#3821142, @davidxl wrote:

In D134152#3820125, @nikic wrote:

It's fine to work around this in the meantime (it is a correctness problem when it comes to mustprogress at least) -- but shouldn't this be done by dropping the loop metadata? I believe that's the general rule for metadata: If a transform cannot safely preserve metadata, this is always resolved in favor of dropping the metadata, not preventing the transform.

Dropping the metadata in this case changes the program semantics (unlike dropping profile data), thus the transformation in this case is unsafe so it should be disabled as this patch does.

I think I am missing what the issue with dropping the loop metadata would be, could you elaborate? If we drop the metadata we should only lose information that may have enabled additional transformations, but shouldn't change the result of the program.

In D134152#3820125, @nikic wrote:

It's fine to work around this in the meantime (it is a correctness problem when it comes to mustprogress at least) -- but shouldn't this be done by dropping the loop metadata? I believe that's the general rule for metadata: If a transform cannot safely preserve metadata, this is always resolved in favor of dropping the metadata, not preventing the transform.

Perhaps add a TODO in the comment part for the longer term solution suggested by nikic. Otherwise LGTM

lgtm

Jumpthreading 'avoids' the problem with an earlier guard. Since the fix is in a common path, it is ok to just test simplifyCFG.

Re nikic's comment -- I agree with the statement in general, but I think the infrastructure fix is orthogonal to the bug fix (with the status quo). For what you suggested, we should probably file a bug to track it.

Thanks for the nice graph. It looks like after the simplification, for.cond is used as latch for both the inner and outer loop, but only one MD data is allowed. In this case, I think we should disable the block merge.

lgtm

This looks like related to infinite loops.

It would be clearer to describe the problem with example CFGs before and after (probably using ascii art) and explain how the meta data is lost during this transformation.

lgtm

lgtm (The InlineFunction body is getting really big. Probably worth breaking it up in the future).

Why not adding another parameter to InlineFunction method to specify whether attribute Merge is done?

lgtm

Ok -- my confusion was the naming of the SinkPt variable -- it should actually be named 'SI'. I through the sinkpt is the actual bb after select is expanded into branch.

It is unclear that the more precise life time tracking can make a big difference given the code region (post transformation) is small, so this handling LGTM.

The example provided shows that there is a missing opportunity for CSE (of address computation of Vec[i]). The decision on GEP ordering should probably better be based on more general reassociation analysis for enabling more CSE/LICM.

The patch looks reasonable to me. Check if @fhahn @RKSimon have further comments.

Can you simplify the test case (e.g. stripping the debug info)?

lgtm

lgtm

I did not look at the patch in details. At a high level, it seems that the pass itself does not use profile data to make decisions, so BPI/BFI is really not needed. Instead, directly updating the profile meta data as transformation happens is reasonable (unless BFI provides updating APIs for cases like this, which is not the case).

emit an error for merging profiles of different kind is ok.

lgtm