lgtm

LLC miss info should be very sparse, so mixing it with LBR may not bring a lot of benefit. Having mixure of different types of profile data can lead to combinatorial growth of the number of readers. As the number of profile types increases, it can cause the reader to be unmaintaintable. That is why I suggest making it clean -- one type per section.

llvm-profdata should also have a round trip test:

Can you split the format extension change into a separate patch?

static prediction is intended to help common use cases, so for corner cases when NaNs are used frequently, user will need to resort to source annotation (such as builtin_expect).

lgtm

See Some minor name suggestions, also

This version looks fine to me, but please let other reviewers to weigh in as well.

lgtm

I suspect other affected cases are due to bad static profile data too.

I did some test with the patch with samplePGO and it works fine. LGTM

It is also better to introduce a coldness factor parameter 'F', i.e, if the source block's count is less than 1/F of the header count, suppress the hoisting.

Hal probably remembered more details. IIRC, the argument was that LICM provides IR canonicalization which can simplify analysis -- there were some examples about exposed vectorization opportunities etc.

This was tried before. IIRC, the conclusion was to implement look sinking pass to undo non-profitable LICM. The loop sinking pass was in D22778. Can the loop sinking pass be enhanced to handle the case here ( I have not looked in details) ?

carrot, can you help looking into this? The cost model should look into extra direct jumps introduced as well.

Is the test case slow down with PGO or not? Also do you have branch misprediction perf data? (large slowdowns like this is usually triggered by side effect like this). Is there a trimmed down version to demonstrate the issue?

Can you put an example (text CFG) showing cases where tailDup is bad -- possibly as a source comment?

lgtm

lgtm

Some end-to-end tests as in compiler-rt are probably helpful here.

lgtm

Is it possible to split the patch into a NFC refactoring and one functional change?

lgtm

lgtm

please flip the flag to false by default and check in the rest first.

my test hit some other unrelated debug assert. Can you first change the debug assert into error message so that there is no need to use assert enabled build to test the option?

Does the bursty style sampling provide additional benefit? A non-bursty style sampling can be cheaper:

Do you have a functional test case to show what this patch is trying to accomplish/fix?

let me do some internal PGO build tomorrow with the option on and get back.

What types of testing has been done ? I assume the code base now is relatively clean so it is safe to turn this on by default?

Rong, can you take a look at this patch?

The refactoring part looks ok. +vsk to double check.

Is there a bug reference somewhere?

lgtm

llvm-cov's user manual probably also needs some update.

is thin archive supported?

lgtm

This can be handled this way:

Warning. With static profiling, the layout strategy based on the 'precise' cost model may be off. If for some reason this causes issues later, the change should be guarded with 'hasProfile' check.

It is just more convenient for you to contribute to LLVM in the future. I can help you commit this time.

I suggest you apply for commit right first. There are likely fallouts that need to be dealt with.

lgtm. Watch out for problems on other platforms.

lgtm assuming the state tracking code gets removed (or put under NDEBUG in some way) after the cleanup.

For all the test cases update, please also validate if they make sense or not if possible.

once the cleanup is done, the state tracking should be removed as well.

I suggest the following path:

1. add an option to enforce internal error on inconsistency; by default the inconsistency is ignored.
2. fix the exposed root cause one by one
3. turn on the option by default

Sounds reasonable -- the intention is clear with this -- the interface tells runtime to do whatever needed to ensure merging. Please send a patch with merging test case.

What I meant is that we should never see inconsistent state and instead of setting invalid state, we assert there. Ideally, the profile setting/update APIs should help user avoid creating the inconsistent state in the first place (i.e., catch the problem on the spot).

I think it is cleaner for user to make the intention clear by passing the flag if the file handle is passed to the runtime. Otherwise the runtime will have to guess/assume user's intention (e.g, also reopen the file with "r+b" mode for merging).

To support merging, the new interface needs to take more

Why is symbol remapping an issue? old MD5 --> input symbol name -> output symbol name --> mapped MD5sum ?

what is the main blocker for the longer term solution?

Longer term, the plan is to put a white list of the symbols into the profile data so that the compiler can decide if a function is newly created or simply cold.

There should be a test case.

lgtm

Does this patch handle the case in D62079? If yes, you can move the tests added in that patch here.

lgtm

Indeed. The original one is O(N^2). LGTM.

was there a quadratic behavior before ? It seems linear before and after the patch -- but just cut the computation by a factor of 2 . Do you need an option to control the max chain length?

Loop rotation not only affects fall through placement of incoming and exit edges, but internal fall through as well -- it converts the original backedge into a new fallthrough while converts an original internal fallthrough into the backedge. In other words, the overall cost/benefit needs to consider all changes. Doing pattern matching like this can lead to wrong decisions.

I like the direction this patch is going, but I think it can do better.

lgtm

lgtm

lgtm

lgtm

it is probably better to put the tests in the same file with different check prefixes.