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.

lgtm

lgtm

lgtm

lgtm

Update related interfaces in ProfileSummaryInfo.

It is tempting to use enum in this case, however I don't see possible extension to the type (in forseable future), so perhaps keep this way.

Updated the patch to include more APIs.

This is not completely NFC as it modifies branch weight update behavior (though correctly), so please do more pgo testings to avoid surprises (e.g, some weird dependencies on old behavior).

Looked at the Instruction::updateProfWeight() --- the part that update branch_weights seems bogus -- there is need need to scale branch weight at all.

Can you clarify? IIRC, we never update the per-call site profile meta data with inline transformations.

It is possible to for an entry with zero count -- as for instance in instrumentation PGO. Should it be fixed in place where the div by zero happens?

lgtm

lgtm

Look fine in general. Easwaran can help with review of pass pipeline changes

Missing unit tests for the feature.

lgtm