User Details
- User Since
- Aug 20 2015, 4:19 PM (422 w, 1 d)
Aug 16 2023
lgtm
Jul 20 2023
LGTM.
How about we get rid of the module field? The function passed in all associating with the module so we can get the module from there. And there seems only one use of M (when calling annotateValueSite).
Other changes look good to me.
Jul 17 2023
looks good to me.
Jul 13 2023
It seems that we now need to specify "-mllvm -mfs-psi-cutoff=0" to enable the MFS for FSAFDO, if we want to get the same optimization behavior. This is not good for usage adoption.
I think if the users want to enable MFS in FSAFDO, they should only need to add -fsplit-machine-functions. They should not need to use ANY extra internal options.
Jul 12 2023
LGTM. Thanks for the cleanup!
Jul 5 2023
LGTM.
LGTM.
Jun 9 2023
Apr 24 2023
Thanks for working on this.
PGOEdge is shared by PGO instrumentation pass and PGO profile-use pass. We cast away const for PGO instrumentation. But for PGO profile-use, we don't change the BB. I would prefer to keep the const for profile-use passes. So I think a better fix is to make PGOEdge a template class, one with const and one without.
Apr 20 2023
LGTM.
Mar 9 2023
Mar 8 2023
Integrated review comments/suggestions from Wenlei and Hongtao:
(1) remove BBSize from hash
(2) use less expensive hash function
(3) better names for getSubprogramLinkageName()
The performance for removing BBSize from discriminator hash is out: it shows a slightly gain on performance, like ~0.2% vs with BBsize in the hash. I will remove BBSize from the hash.
Mar 6 2023
Integrated Hongtao's review suggestion.
Thanks Hongtao for the reviews and comments. I'll update the patch shortly.
Mar 2 2023
Mar 1 2023
Yes. This patch is obsolete now.
Feb 27 2023
Feb 24 2023
Feb 16 2023
LGTM.
I did a perf test and I think this is fine. LGTM.
Feb 6 2023
I think this is the reasonable place for CHR.
With this we will do a lot less CHR, we need to run a performance test to ensure there is no regression.
Yes. D143424 is actually what I wanted to do in the following up patch. We need to do some performance test though.
Dec 15 2022
I have some high level questions:
(1) have you considered removing samples with smaller values across the program-- it's like downsampling. Comparing to removing functions with smaller total counts, I think that results in a more consistent profile.
(2) if we choose to remove function, and you sort the function with total count, should we find the exactly place to cut to satisfy the size limit? In theory it should as the profile organized in unit of function. You can keep writing to the buffer until it reaches the limits. Of cause there are some section data for extbinary and summary, but they should be able to compute. Using heuristic to guess the function to remove and doing it iteratively does not seem to be appealing here.
Dec 7 2022
I kind of agree on your point. I also think ModuleOPtimization pipeline
fits better for this pass. This pass is very aggressive in terms of control
flow. In my cases it duplicated the whole function.
Dec 2 2022
Add comments in the tests suggested by Teresa.
Nov 30 2022
Followed Reid's suggestion to explicitly check the IR after the pass.
Nov 29 2022
Nov 28 2022
Removed lambda updateSampleMap() -- it was from the early version and now it was no longer needed as we just had one caller.
Addressed David's review comments.
Simplified the patch a bit by refactoring the existing interface.
Nov 22 2022
Nov 21 2022
Nov 18 2022
Nov 2 2022
Nov 1 2022
Add a test case, per David's suggestion.
Oct 23 2022
Oct 21 2022
Oct 20 2022
Oct 12 2022
Looks good to me.
Oct 7 2022
Oct 6 2022
Sep 21 2022
Sep 19 2022
This is an interesting example. When we design this, we assume there is at least one exit node in the function. And we create fake edges from exit node(s). This way we create a close graph.
I'm fine with change. But can you add a warning message here? I would be helpful to know something is wrong here.
Sep 7 2022
Sep 6 2022
Integrated Hongtao's suggestion.
Sep 5 2022
Integrated with review comments.
Sep 1 2022
Aug 31 2022
I'm fine with this change.
Aug 29 2022
I agreed with Teresa: adding an optional string is better than using a separated metadata here.