LGTM

This looks good to me. But I'll let other approve this.

Integrated David's review comments.

The transformation part looks good to me.
Func validateRecord() still checks entries with 0 size only.
But it's OK, as the transformation will catch bad profiles.
I'll approve this patch. Sorry for taking so long.

Integrated the comments from David Blaikie.

In D96981#2572663, @dblaikie wrote:

Really really broad feedback - this work to write common abstractions over LLVM IR and MIR seems to overlap with https://lists.llvm.org/pipermail/llvm-dev/2020-December/147433.html

It's non-trivial as to what exactly these APIs should look like (virtual/type erased v templates, etc). But may be useful to consider some of the options here.

My current rough guess is that templates are probably the right tool, but that more common abstractions should be implemented in the IR types themselves - eg: the TypeMap and getFunction/getEntryBB could be rolled into the IR types themselves as authoritative/core features, rather than grafting them on with traits.

Adding "inline" keywords will make the functions COMDAT and will fix the
duplicates problem. I verified with -DLLVM_ENABLE_MODULES=On.

Thanks for reporting this.
What config this failure is using? I don't see this error in my builds.
This is an intermediate step for templatelizing the code. I don't think
this will be a problem for the templated headers.

Yes. I'm aware of this. I had a fix already. Testing it now.

yes. I'm working this. I will send a patch shortly.

No worry. This fix also works for me. Thank you and Kazu for the quick
response! Much appreciated.

Thanks for fixing this. I think this is fine. I don't think there will a direct allocation of the base type. We always use the derived type to allocate the object.
Samples field will be freed by the Reader which is an uniqe_pointer.

Thanks for the update!
I agree that the probe based part can be moved to the base later. Once all
the lineno based code is in place, moving code should be straightforward.

WeiLei and Hongtao, do you have any concerns / comments? If not, I will
commit this change today.

Here is the version include the definition of SampleCoverageTracker in the base class.
Without using the wrapper functions, it does look a bit cleaner.

Integrated Wei's review comment.

This looks fine to me. Thanks for fixing.

Integrated Wei's review comments.
The test case turns out to be my problem. Also fixed it.

Thanks, Wei!

Thanks David for explaining this for me. Sorry for not making it clear in
the original email.

Here is the another way to refactor the SampleProfileLoader:

I meant the functions move the header file. In the header file, we can
achieve the same by adding an inline keyword to the out of class function
definition (vs in class definition). Out of class definitions have better
readability because some of the functions are really large.

Machine level profile loader will use the SampleCoverage Tracker. This
class will be kept in the templated header. Machine level profile loader
and IR level profile loader will be using the exactly the same class (not
templatelized).

It's only used in this file, by SampleProfileLoader and SampleProfileTracker class. I could make it a class static method (in SampleProfileLoader), But I don't see an advantage of that -- just with longer class qualifier.

I think the patch is going the right direction. Some minor comments:
(1) If we have two entries with the same value (other than 0), will we have the same issue? I think we will have same invalid switch statement. So can we extend this patch to handle this? That should be a small change.
(2) can we change "invalid merge" to "invalid profile"? This does not look like a merge issue -- at least from the test case where the problem is from the raw profile. This points to profile runtime issue. I think invalid profile is more appropriate here.

lgtm too

Sorry for the delay.

looks good to me. Thanks for working on this.

yes. Also #43 should be #42
I fixed it in
https://reviews.llvm.org/D92493
But that was just to make my test pass.

Integrated comments from David and MaskRay.

This breaks llvm/test/Bitcode/attributes.ll.
The added test code was obviously wrong.

Thanks for catching that.
I messed up the commit from another patch.
Fixed in commit c36f31c.

Updated LangRef and added test cases per review comments.

Sorry to the late response (I somehow missed the reply).

Look reasonable to me.

I'm not sure llvm-profdata is good place to fix. I would rather to fix this in the memop size transformation -- to detect this in the heuristic.
At the same, I'm interested to know what is the cause of this. Should we do something in profile runtime to prevent this.

Integrated David's review comments.

Using remarks to print the message.
Note that the message printing is still under the internal option.
(This seems OK to me as other options, like EmitBranchProbability, are doing the same.)

LGTM.

Split the verification to another patch.

Updated the patch that addressed David's review comments.
One noticeable change was to .add a new option "pgo-vefify-hot-bfi" which will
output a message when one of the following happens:
(1) a hot rawCount becomes non-hot in BFI
(2) a cold rawCount becomes hot in BFI

I'm reviving this patch because this fixes one of our internal bugs.

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz (SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the preinliner is pretty conservative. But there will be cases size will be increased.

Yes. This looks a typo to me too.

Update the patch after committing the support in a separated patch:
commit 50da55a58534e9207d8d5e31c8b4b5cf0c624175