User Details
- User Since
- Aug 20 2015, 4:19 PM (353 w, 1 d)
Apr 7 2022
Also, adding a test would be helpful.
Mar 8 2022
Feb 25 2022
Feb 24 2022
Another way to do this is to check PGOOpt->DebugInfoForProfiling in TargetPassConfig before adding the passes.
That would be the same as IR level addDiscriminator.
Feb 23 2022
LGTM.
Thanks for this fix. I kind of know this (I used to have an assert to after calling SplitIndirectBrCriticalEdges(), but got rid of it because some cases it does return false).
Even this fix, there are still chance that we cannot split the edge.
Maybe we should emit a warning in this case.
Feb 22 2022
I think the reason MaxNumPromotion==0 breaks is likely the 0 size array think (like MaskRay mentioned).
Feb 5 2022
Feb 4 2022
Dec 9 2021
Nov 23 2021
This looks good to me.
Nov 22 2021
Nov 18 2021
Oct 19 2021
Sorry. I missed that part.
Oct 14 2021
Integrated Reid's review comment.
Oct 13 2021
Reid: thanks for pointing out the error for MSC. I move the Macro definition to compiler.h.
Oct 12 2021
Sep 16 2021
Wei committed the FSAFDO tool part of change to GitHub yesterday. Sorry for
taking so long.
Sep 3 2021
Aug 25 2021
Thanks Wenlei for the fast response!
Aug 24 2021
Thanks for fix this!
I like this fix and the patch looks good to me.
Aug 23 2021
The problem you described is exactly what we wanted to solve with comdat renaming.
We had some correctness issues so I put many restrictions here. We have switched the default linker to lld
and linkage handling has evolved quite bit. I might revisit some of
the restrictions again, like if we can enable the renaming for weak linkage.
Added a few comments to the test cases as suggested by Fangri.
Thanks to Fangrui for the comments/suggestions.
Integrated his reviews to the patch.
Fixed
This is still problematic because we currently query this symbol to coordinate some actions B/W PGOInstrumentation and InstroProfiling lowering, like whether to do value profiling, weather to do comdat renaming.
Worth clarifying B/W.
InstruProfling
InstrProfiling
Fixed
I have confirmed that
In regular LTO, the non-prevailing variable is dropped unless its linkage weak_odr/linkonce_odr && -compute-dead=0
In ThinLTO, the variable is changed to available_external linkage by thinLTOResolvePrevailingGUID then dropped by GlobleOpt:deleteIfDead in the backend.
Thanks for confirming and put the conditions here
I sent my version of patch as https://reviews.llvm.org/D108581
Aug 20 2021
This looks fine to me. But I agree you that a more complete fix would be having CurLinkModule set for all callers.
Sorry that I did not follow this patch closely. But after reading this I don't think this is a proper fix.
Thanks for the fix!
Aug 18 2021
Discussed with Wei offline: it's better to set fs profile loader as off by default so people can opt-in (rather opt-out).
Once we are done with more perf testing, we will switch the default to on.
Yes. Resetting them will not cause correctness problems.
Fix some issues in last patch set.
I just sent out the patch that puts PGOOptions to TargetMachine before
getting this message.
So EnableFSDiscriminator is not included.
Followed Wei's suggestion to put PGOOptions to TargetMachine.
Aug 17 2021
Integrated recent comments from Wei and Hongtao.
Aug 16 2021
Integrated review comments from Wei, Wenlei and Hongtao. Thanks for all the suggestions/comments!
Aug 13 2021
Thanks all for the review comments! My replies are inlined.
Aug 12 2021
Aug 11 2021
Aug 10 2021
Add another pass berfore RA (as suggested by WenLei).
I tested the performance for FSAFDO Loader using one google benchmark.
This extra pass turns to be positive in performance -- I'm seeing additional 0.6% - 0.8% improvement.
Jun 21 2021
Integrated Wenlei's suggestion to remove the redundant interface.
CSPGO can have mismatches -- it's just like PGO that is triggered by a source change, or a compiler change (before the pass).
Usually if there is a mismatch, PGO use pass will catch it first. There are cases that PGO has no-mismatch while CSPGO has a mismatch, but I consider it extremely rare.
Jun 20 2021
Sent a wrong patch.
This is the patch I wanted to send for review.
Jun 18 2021
This might just be specific to our deployment. Our AFDO optimized build
uses the profile generated in the previous release.
If we hold this Pass1 discriminator change and commit together with the FS
profile change. We won't see the FSAFDO performance in the release right
after -- we need to wait for another release to get all the samples with
Pass1 discriminations.
Jun 15 2021
@Fangrui: My initial version was to place it into the use list. But Hoy
suggested that I use CommonLinage.
From llvm reference manual, it seems that linker should GC a common symbol.
Can you read the message and add comments there?
https://reviews.llvm.org/D103988
Jun 11 2021
Jun 9 2021
Jun 4 2021
Integrated Hongtao's suggestion to move setDiscriminatorMaskedBitFrom() to SampleProfileReader::create()
Jun 3 2021
Per Wei's suggestion, using enum option.