- User Since
- Aug 20 2015, 4:19 PM (271 w, 1 d)
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:
Jul 24 2020
Jul 23 2020
Jul 22 2020
Addressed review comments from David.
Jul 21 2020
Jul 17 2020
Changed the implementation based on review comments.
Now we will not change the version of the index profile.
We will use bit 58 in the profile header to indicate if
we always instrument the entry block. This applies
to both raw and index format.
Jul 1 2020
Jun 26 2020
Jun 18 2020
May 29 2020
Thanks for the fix. We also noticed that the branch weight for invoke instructions were not set properly, even thought we had the profile information in PGO.
I have a check that disables setting the branch weight metadata for IRPGO. Once this patch is in. I can remove that check. Of you can integrate that change into this patch.
It's in PGOUseFunc::setBranchWeights(). Just white-list InvokeInst.
May 19 2020
I noticed the same issue when I implemented CSFDO -- I initially reused the same summary name for CSFDO as in FDO.
Exiting of two module flags with the same name sometime breaks the build (not always, if I remember correctly).
Mar 23 2020
look good to me with the fix of comments.
Mar 5 2020
One correction to my previous comment: the default of NOT COMPILER_RT_ENABLE_PGO is true. But I think it's OK, as far as we have a way to build the instrumented version of compiler_rt.
The problem here is libclang_rt.fuzzer-x86_64.a is instrumented and contains calls to profile runtime functions like llvm_profile_instrument_target.
When building with fuzzing, libclang_rt.fuzzer-x86_64.a is linked in. We also have the definition of llvm_profile_instrument_target in the compiler.
But I don't think the compiler knows how to resolve this symbol to itself.
Mar 4 2020
From crbug:1018840, it looks libfuzzer does not work with instrumentation. Do you know the reason.
I tried clang PGO bootstrap that enables PGO instrumentation in compiler-rt for stage2 compiler. Everything works fine -- i.e. profile runtime functions in compiel-rt generate a correct profile.
Feb 25 2020
Thanks for working on this. Here are a few comments:
(1) I'm not sure why PGO options are passed to compiler-rt. It's probably harmless for -fprofile-use or -fprofile-instr-use as we only use the profile. My concern is we might instrument profdata runtime routine. This can create recursion that leads to run time issue.
Feb 24 2020
Uploaded a wrong patch in the last time.
This is the correct one.
check if the -fdiscard-value-names explicitly in args (suggested by Jeroen and Serge)
Gentle ping. Is the newest patch ok?
Feb 21 2020
Update the patch based on the reviews from Mehdi and Serge.
Reuse Sege's warning code in https://reviews.llvm.org/D74871?id=245594.
Feb 20 2020
Feb 19 2020
Jan 28 2020
Thanks for working on this. Current Duplication in block-placement does need improvement. We notice that disable it often improved performance.
Jan 14 2020
Jan 13 2020
Thanks Teresa for the suggestion: using the print() function in the llvm diagnosticPrinter is a lot better than duplicating the code. I changed the patch to use print() function. Also added comment as suggested by Teresa.
Integrated Teresa's review comments.
Jan 10 2020
Oct 10 2019
I totally agree with what eugenis said.
I added his comments to a TODO comment.
I will commit this version and may address the TODO later.
Oct 2 2019
Thanks for improving this.
Oct 1 2019
Sorry it takes long to review this.
I have a pending patch that value profiling the expensive MOD/REM instructions. It is more tightly coupled to edge profile (than current mem-size-op and icall value profile).
The difference is that it does not have <valuesite, value> pair. The counts are stored like the edge counts.
It seems to be fine with patch as the instrumentation says in PGOInstrumenation.cpp and only the candidate collecting is extracted out.
Sep 30 2019
Sep 25 2019
Fixed comments suggested by Guillaume.
Sep 24 2019
Sep 13 2019
For the legacy pass manager, use option "-mllvm -debug-pass=Structure", or in the source, we add the lowering pass right after instrumentation pass.
LGTM. Thanks for the fix!
Aug 2 2019
Aug 1 2019
Jul 29 2019
Integrated Chandler's review comments.
I'm sorry that I missed this review for this long!
Jul 9 2019
Jul 8 2019
Change the error report format suggested by David.
Jul 3 2019
Jul 1 2019
Sent the wrong test file in last patch.
Update to the correct one.
Jun 28 2019
they are not doing the exacly the same thing for old pass manager and new pass manger: old pass manger is doing instrumentation, while the new pass manager with this change is NOT.
the test is not check instrumentation, (it only check the variables that generates by InstroProfiling pass).
In this sense, the test is not well written.
This patch does not make sense to me.
Jun 10 2019
Integrated David's review suggestions.