Yes. This looks a typo to me too.

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

Addressed review comments from David.

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.

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.

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).

look good to me with the fix of comments.

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.

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.

LGTM

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.

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?

Update the patch based on the reviews from Mehdi and Serge.
Reuse Sege's warning code in https://reviews.llvm.org/D74871?id=245594.

In D74878#1884769, @serge-sans-paille wrote:

@lebedev.ri maybe we should at least warn the user if there was a flag at clang level to force discarding? That's what I've been doing in https://reviews.llvm.org/D74871?id=245594

Thanks for working on this. Current Duplication in block-placement does need improvement. We notice that disable it often improved performance.

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.

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.

Thanks for improving this.

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.

Fixed comments suggested by Guillaume.

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!

Integrated Chandler's review comments.

I'm sorry that I missed this review for this long!

Ping

Change the error report format suggested by David.

In D63949#1569175, @davidxl wrote:

Does the bursty style sampling provide additional benefit? A non-bursty style sampling can be cheaper:

sample_count++;
if (sample_count == SamplePeriod) {

increment_prof_counter();
 sample_count = 0;

}

Sent the wrong test file in last patch.
Update to the correct one.

In D63155#1563266, @chandlerc wrote:

In D63155#1563240, @leonardchan wrote:

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass
MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

As an alternative, could I instead request that we remove the BackendUtil changes and just mark the runs in gcc-flag-compatibility.c with -fno-experimental-new-pass-manager. This way we could clarify that under the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.

No, I think we should be doing PGO at O0 in the new PM if we do so in the old PM.

I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct to fix this test case. That seems plausible to me at least, and I think reverting and figuring out what the right way to do it is a fine approach.

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.

Integrated David's review suggestions.

In D62700#1536841, @davidxl wrote:

This can be handled this way:

BBInfo &SrcInfo = getBBInfo(SrcBB);    // BBInfo is the template type
BBInfo &DestInfo = getBBInfo(DestBB);
SrcInfo.addOutEdge(E.get());
DestInfo.addInEdge(E.get());

For base class BBInfo, addOutEdge, addInEdge will be empty, while it is defined for UseBBInfo class.

By so doing, the code is more readable.