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.

Integrated David's review comment.
For the merge profile merge, also skip the value profile merge if the count is pseudo kind.

Integrated David's most recent review comments.

Integrated Sriraman's suggestion.

This is more complex change that hide the pseudo count values in InstroProfRecord.
Needs to touch more files. But I think this improves the original patch.

Fixed unneeded &

This patches adds the support for mapping to warm function, just following the existing practice (where it maps to hot functions).
The constant of -1 is already in llvm.

Integrated David's review commends.