lgtm

LGTM.

How about we get rid of the module field? The function passed in all associating with the module so we can get the module from there. And there seems only one use of M (when calling annotateValueSite).
Other changes look good to me.

looks good to me.

It seems that we now need to specify "-mllvm -mfs-psi-cutoff=0" to enable the MFS for FSAFDO, if we want to get the same optimization behavior. This is not good for usage adoption.
I think if the users want to enable MFS in FSAFDO, they should only need to add -fsplit-machine-functions. They should not need to use ANY extra internal options.

LGTM. Thanks for the cleanup!

LGTM.

In D152399#4404698, @wenlei wrote:

we see a 1.2% performance improvement in clang benchmark, 0.9% QPS improvement in our internal search benchmark, and 3%-5% improvement in internal storage benchmark.

This looks promising. We saw improvements from MFS when used with CSSPGO too, but the improvements were smaller than the numbers here. You are not using Propeller or BOLT in your perf testing above, right?

What is the baseline in your experiments? are you comparing 1) FS-AFDO w/ MFS vs FS-AFDO w/o MFS, all using this change; or comparing 2) after vs before this patch, using FS-AFDO + MFS on both sides?

+@hoy @wlei

! In D152399#4407001, @shenhan wrote:
What is the baseline in your experiments? are you comparing 1) FS-AFDO w/ MFS vs FS-AFDO w/o MFS, all using this change; or comparing 2) after vs before this patch, using FS-AFDO + MFS on both sides?

Thanks. I was comparing "1) FS-AFDO w/ MFS vs FS-AFDO w/o MFS". And FS-AFDO profile is collected via -improved-fs-discriminator=true.

LGTM.

In D148876#4289423, @davidxl wrote:

In D148876#4289326, @hoy wrote:

Thanks for the illustration. For AutoFDO, I took a look at the debug info merger (https://github.com/llvm/llvm-project/blob/a17b71d17f853350dcd6c72ab141b196d7caec2a/llvm/lib/IR/DebugInfoMetadata.cpp#L114). Here is my understanding:

1. For two callsites that are originally from different lines of a function, the merged location will have a zero line number. This basically means the value profile for the merged callsite will be dropped by the sample loader.

Thanks for checking. I suspect bugs like this may exist, but it is orthogonal to the problem at hand.

Thanks for working on this.
PGOEdge is shared by PGO instrumentation pass and PGO profile-use pass. We cast away const for PGO instrumentation. But for PGO profile-use, we don't change the BB. I would prefer to keep the const for profile-use passes. So I think a better fix is to make PGOEdge a template class, one with const and one without.

LGTM.

In D145171#4179636, @hoy wrote:

In D145171#4179269, @xur wrote:

The performance for removing BBSize from discriminator hash is out: it shows a slightly gain on performance, like ~0.2% vs with BBsize in the hash. I will remove BBSize from the hash.

Good to know removing it helps perf. Can you please update the summary as well? LGTM otherwise.

Integrated review comments/suggestions from Wenlei and Hongtao:
(1) remove BBSize from hash
(2) use less expensive hash function
(3) better names for getSubprogramLinkageName()

The performance for removing BBSize from discriminator hash is out: it shows a slightly gain on performance, like ~0.2% vs with BBsize in the hash. I will remove BBSize from the hash.

In D145171#4174356, @wenlei wrote:

This reduces the hash conflicts.

Curious how did you check/detect conflicts/collisions?

Integrated Hongtao's review suggestion.

Thanks Hongtao for the reviews and comments. I'll update the patch shortly.

Yes. This patch is obsolete now.

LGTM.

I did a perf test and I think this is fine. LGTM.

I think this is the reasonable place for CHR.
With this we will do a lot less CHR, we need to run a performance test to ensure there is no regression.

Yes. D143424 is actually what I wanted to do in the following up patch. We need to do some performance test though.

I have some high level questions:
(1) have you considered removing samples with smaller values across the program-- it's like downsampling. Comparing to removing functions with smaller total counts, I think that results in a more consistent profile.
(2) if we choose to remove function, and you sort the function with total count, should we find the exactly place to cut to satisfy the size limit? In theory it should as the profile organized in unit of function. You can keep writing to the buffer until it reaches the limits. Of cause there are some section data for extbinary and summary, but they should be able to compute. Using heuristic to guess the function to remove and doing it iteratively does not seem to be appealing here.

I kind of agree on your point. I also think ModuleOPtimization pipeline
fits better for this pass. This pass is very aggressive in terms of control
flow. In my cases it duplicated the whole function.

Add comments in the tests suggested by Teresa.

Followed Reid's suggestion to explicitly check the IR after the pass.

Removed lambda updateSampleMap() -- it was from the early version and now it was no longer needed as we just had one caller.

Addressed David's review comments.

Simplified the patch a bit by refactoring the existing interface.

In D138332#3938399, @tejohnson wrote:

Where is the CSFDO annotation step in relation to this pass? If it would be annotated before this point on the subsequent CSFDO use compile, is the idea that we might as well wait for the CSFDO profile before performing post-link CHR?

Presumably even if the profile is the same as the pre-link CHR, the code could be different after cross-module importing, inlining, other cleanup, so wouldn't that open up more opportunities potentially for CHR?

Add a test case, per David's suggestion.

In D137184#3900121, @wenlei wrote:

Why is number of critical edges (as opposed to IR size or number of blocks) a better indicator for function size related build speed issues?

Looks good to me.

In D135427#3842972, @tejohnson wrote:

Regular LTO is not fixed yet.

Looks like handleNonPrevailingComdat in LTO.cpp would need a similar fix.

When FuncImport.cpp ...

Note that while this functionality currently lives in FunctionImport.cpp it isn't really related to function importing, rather to ThinLTO symbol resolution. So mentioning the file name might be a little misleading (but if you do, correct it to FunctionImport.cpp).

@xur can you confirm this fixes the original problem?

In D134203#3801212, @xur wrote:

This is an interesting example. When we design this, we assume there is at least one exit node in the function. And we create fake edges from exit node(s). This way we create a close graph.

For this case, we don't have an exit node and I think the function exits from a non-return call to "zeroexit". So the edge propagation will terminate incompletely.

I think this is a corner case but let me think how to handle this.

This is an interesting example. When we design this, we assume there is at least one exit node in the function. And we create fake edges from exit node(s). This way we create a close graph.

I'm fine with change. But can you add a warning message here? I would be helpful to know something is wrong here.

In D134203#3800684, @davidxl wrote:

This looks like related to infinite loops.

In D133121#3775072, @hoy wrote:

In D133121#3775043, @xur wrote:

In D133121#3774815, @hoy wrote:

In D133121#3774799, @xur wrote:

In D133121#3774764, @hoy wrote:

In D133121#3774657, @xur wrote:

In D133121#3772956, @hoy wrote:

Thanks for the change. Wondering if you saw performance improvement with it.

The corresponding change on the sample profile side isn't included, do you have any plan for it?

Yes. For AutoFDO, we still need some change for the invoke in sampleloader
The situation is slightly different in AutoFDO. In FDO, we do branch_weight annotation first, then do the VP.

In AutoFDO, we do VP annotation first and then branch_weight annotation. We need to set the branch_weight metadata where there is already a VP metadata (which is the reverse of FDO).
I was planning to have a separated patch for AutoFDO, but I can also include it in this patch.

A separate patch sounds good, thanks.

AutoFDO is less problematic as branch_weights metadata overwrites VP in the end. (and many VP metadata is already consumed by the prompting in preinliner).

In FDO, VP metadata overwrites branch_weights -- This is bad as we lost more information in BPI/BFI.
(branch_weights is more important for downstream opt).

It looks like same happens to autofdo, i.e, no branch_weights assigned to indirect invokes? https://github.com/llvm/llvm-project/blob/7b81a81d5f9cccb1b091cfc5264bc483b0acc83a/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1628

Oh, I guess branch_weight is attached to invokes that are promoted from an indirect invoke, so it is better handled than the FDO case.

the branch weight for invoke is set in line 1724 (https://github.com/llvm/llvm-project/blob/7b81a81d5f9cccb1b091cfc5264bc483b0acc83a/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1724)

line 1628 sets the call's branch-weight (which is a single number, call counts) -- this should be overwritten by line 1724.

Line 1724 is guarded by !TI->extractProfTotalWeight(TempWeight) which is non-zero for invokes with !VP?

In D133121#3774815, @hoy wrote:

In D133121#3774799, @xur wrote:

In D133121#3774764, @hoy wrote:

In D133121#3774657, @xur wrote:

In D133121#3772956, @hoy wrote:

Thanks for the change. Wondering if you saw performance improvement with it.

The corresponding change on the sample profile side isn't included, do you have any plan for it?

Yes. For AutoFDO, we still need some change for the invoke in sampleloader
The situation is slightly different in AutoFDO. In FDO, we do branch_weight annotation first, then do the VP.

In AutoFDO, we do VP annotation first and then branch_weight annotation. We need to set the branch_weight metadata where there is already a VP metadata (which is the reverse of FDO).
I was planning to have a separated patch for AutoFDO, but I can also include it in this patch.

A separate patch sounds good, thanks.

AutoFDO is less problematic as branch_weights metadata overwrites VP in the end. (and many VP metadata is already consumed by the prompting in preinliner).

In FDO, VP metadata overwrites branch_weights -- This is bad as we lost more information in BPI/BFI.
(branch_weights is more important for downstream opt).

It looks like same happens to autofdo, i.e, no branch_weights assigned to indirect invokes? https://github.com/llvm/llvm-project/blob/7b81a81d5f9cccb1b091cfc5264bc483b0acc83a/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1628

Oh, I guess branch_weight is attached to invokes that are promoted from an indirect invoke, so it is better handled than the FDO case.

In D133121#3774764, @hoy wrote:

In D133121#3774657, @xur wrote:

In D133121#3772956, @hoy wrote:

Thanks for the change. Wondering if you saw performance improvement with it.

The corresponding change on the sample profile side isn't included, do you have any plan for it?

Yes. For AutoFDO, we still need some change for the invoke in sampleloader
The situation is slightly different in AutoFDO. In FDO, we do branch_weight annotation first, then do the VP.

In AutoFDO, we do VP annotation first and then branch_weight annotation. We need to set the branch_weight metadata where there is already a VP metadata (which is the reverse of FDO).
I was planning to have a separated patch for AutoFDO, but I can also include it in this patch.

A separate patch sounds good, thanks.

In D133121#3772956, @hoy wrote:

Thanks for the change. Wondering if you saw performance improvement with it.

The corresponding change on the sample profile side isn't included, do you have any plan for it?

Integrated Hongtao's suggestion.

In D133121#3764425, @davidxl wrote:

Can you simplify the test case (e.g. stripping the debug info)?

Integrated with review comments.

In D133121#3765650, @wenlei wrote:

Why is invoke unique here? Can regular call never have both VP and branch weights? Ideally we'd avoid special case for a narrow type of instruction.

I'm fine with this change.

I agreed with Teresa: adding an optional string is better than using a separated metadata here.