- User Since
- Aug 20 2015, 4:19 PM (294 w, 2 d)
Fri, Apr 2
Tue, Mar 23
Mon, Mar 22
Wed, Mar 17
This looks good to me. But I'll let other approve this.
Feb 25 2021
Feb 19 2021
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.
Feb 18 2021
Feb 17 2021
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.
Feb 16 2021
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.
Feb 11 2021
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.
Feb 10 2021
Integrated Wei's review comments.
The test case turns out to be my problem. Also fixed it.
Feb 8 2021
Thanks David for explaining this for me. Sorry for not making it clear in
the original email.
Feb 6 2021
Here is the another way to refactor the SampleProfileLoader:
Feb 3 2021
Feb 2 2021
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.
Feb 1 2021
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
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.
Jan 29 2021
Jan 26 2021
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.
Jan 14 2021
Sorry for the delay.
Jan 12 2021
looks good to me. Thanks for working on this.
Dec 17 2020
Dec 16 2020
yes. Also #43 should be #42
I fixed it in
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.
Dec 14 2020
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.
Dec 8 2020
Sorry to the late response (I somehow missed the reply).
Look reasonable to me.
Dec 2 2020
Dec 1 2020
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.
Nov 20 2020
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.)
Nov 19 2020
Split the verification to another patch.
Nov 18 2020
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.
Nov 17 2020
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.
Sep 11 2020
Yes. This looks a typo to me too.
Jul 27 2020
Update the patch after committing the support in a separated patch: