Also, adding a test would be helpful.

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.

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.

I think the reason MaxNumPromotion==0 breaks is likely the 0 size array think (like MaskRay mentioned).

This looks good to me.

Sorry. I missed that part.

Integrated Reid's review comment.

Reid: thanks for pointing out the error for MSC. I move the Macro definition to compiler.h.

In D111682#3060328, @davidxl wrote:

Does it have the same issue with FE instrumentation? The only thing to consider here is the coverage test may be affected.

Wei committed the FSAFDO tool part of change to GitHub yesterday. Sorry for
taking so long.

Thanks Wenlei for the fast response!

Thanks for fix this!
I like this fix and the patch looks good to me.

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.

In D108581#2961321, @MaskRay wrote:

Thanks for the patch. llvm.compiler.used does look better to me.

---

This happens in the module where IRPGOFlag is mard as non-prevailing

marked

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

In D107034#2959896, @YolandaCY wrote:

@xur This is also reproducible using the existing test case PGOProfile/thinlto_cspgo_gen.ll. If evaluating the generated "%t.2.4.opt.bc" containing non-prevailing IRPGO flag, you will find the value is missed. (The value is still reserved in "%t.2.3.import.bc".)

In the LTO case, the symbol is dropped at *.preopt phase.

In summary after tracing:

1. In LTO, the value is dropped when addRegularLTO as it has external linkage and non-prevailing.
2. In ThinLTO, the value is first changed to available_external linkage by thinLTOResolvePrevailingInModule and thinLTOResolvePrevailingGUID. It is dropped later in the backend GlobleOpt pass by deleteIfDead.

Hence to keep the value we may need to identify the IRPGO variable in all above places, make sure it is reserved, and then drop it after instrumentation to avoid duplicate symbols in later linkage. My feeling is that using a module flag may avoid too much hassle across multiple places and more complexity to the symbol resolution stuff. (as suggested by Reid)
Agree that mixing EnableValueProfiling with IRPGO flag is improper as it seems Clang also may support value profiling (from comment of shouldRecordFunctionAddr ).
Maybe we can rename the module flag to "CSIRPGO"? We can also remove the flag after instrumentation as no need for later passes.

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!

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.

Integrated recent comments from Wei and Hongtao.

Integrated review comments from Wei, Wenlei and Hongtao. Thanks for all the suggestions/comments!

Thanks all for the review comments! My replies are inlined.

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.

In D104579#2831094, @wenlei wrote:

In D104579#2829676, @xur wrote:

In D104579#2828639, @wenlei wrote:

In D104579#2828504, @xur wrote:

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.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

That's a good suggestion. I haven't tried to experiment with one pass before RA. I know that the one before block placement will the performance. I also tried add one after block-placement but did not gain much.
Let me do some experiments and report back.

Another aspect of this change is that we want to do this more conservatively. We have many SampleFDO targets and we don't want to commit too much changes in the initial flip ( to minimize compile time, memory usage impact). We prefer to do this step by step.

Incremental approach sounds reasonable. Was just curious whether you have tried other passes and see no improvements.

Looks like FS-AFDO changes are mostly in tree now though the profile generation part for FS profile is still outside of llvm (in your create_llvm_prof tool), right? Otherwise, we can give it a try too.

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.

Sent a wrong patch.
This is the patch I wanted to send for review.

In D104579#2828639, @wenlei wrote:

In D104579#2828504, @xur wrote:

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.

-Rong

Thanks for clarification, makes sense.

Sounds like your production usage is going to have one FS-Disc pass before block layout. And if we add more FS-Disc pass, say before RA, or others, it's not worth it?

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.

In D103988#2820735, @MaskRay wrote:

CommonLinkage doesn't work.

Whether appendToUsed is needed depends on whether the runtime has a live section referencing __llvm_fs_discriminator__.
If the runtime has the guarantee, no need for appendToUsed.

Otherwise, the first revision appendToUsed with WeakODRLinkage looks good to me.

appendToUsed with ExternalLinkage in a comdat works as well, but comdat will need extra code when Mach-O wants this feature.

@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

Integrated Hongtao's suggestion to move setDiscriminatorMaskedBitFrom() to SampleProfileReader::create()

Per Wei's suggestion, using enum option.