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.

My previous patch was not complete -- we need to set the profile count to the fail-to-instrument edge. Otherwise, we cannot propagate the counts in the CFG.
The new patch has this part of change.
Updated the test to test the profile-use.

Another change of this patch is to move down statistic collection code for critical edge. I think this is a better place.

looks good to me.

Fix comments suggested by David.

Integrated comments from Noel and David.

Adjusted the output string with David's comments.
Also made the naming more uniformed for edge and value profiles.

Discussed offline with David. We changed the output to a simpler: "Description: <Value>" format. This tool will mostly used in a script. Simpler format will make text grep cleaner.

Integrated David's review comments

Integrated David's review comments. Mainly changed the reporting format.

Integrated David's review comments.

LGTM. We need to Initialize the OptLevel no matter what.

Removed some un-intended changes.

Minor fix of the comments.

Integrated David's comments.

In D56675#1426458, @dmajor wrote:

Are there any bugs/reviews/etc. that I can subscribe to for the implementation of this in COFF?

Integrated Ruiu's review comments.

Ping.

using utils/update_test_checks.py to generate the test checks.

changed test case based on spatel's comments.

add one missing updates in last version of the patch.