Address Wei's feedback. Clean up ICPCallee from InlineCandidate now that we don't check InlineCost before ICP.

lgtm, thanks!

lgtm

Thanks for refactoring. We had internal change to add IsFullLTOPreLink alongside with IsThinLTOPreLink for a few places. And we always wanted clean it up too. :)

Thanks for the change.

retitle, rename inlineCandidate.

In D94001#2494594, @wmi wrote:

Sorry for the delay in review. It makes a lot of sense to have a priority based inliner for CSSPGO since its profile annotation doesn't rely on replaying the inlining. But I don't understand why we rely on BFS/DFS strategy to expose the hottest callsite for priority based inliner. In my mind, CSSPGO can know the hotness of each callsite with full context information. Can we just sort all the callsites based on CSSPGO profile then try to inline the hottest callsite under a specific context first in a top down manner? We may actually need to inline some parent callsites before we can inline the hottest callsite, but it is all driven by the target to inline the hottest callsite first. If we worry some callsite is too deep and we need to inline through a deep path before we can inline the specific callsite, we may add the depth into priority computation. What do you think?

In D94001#2479793, @davidxl wrote:

A few high level questions:

1. Can the size bloat problem be handled in llvm-profgen time ? Basically using hotness information to prune/merge profiles properly?

Fix test and linter.

In D92584#2458810, @wlei wrote:

LBRSample will be added when AutoFDO support is moved into llvm-profgen, right? For AutoFDO, we could use the same infrastructure except that context will always be empty.

Good question, Yes, my initial thought is to decouple this by using separated LBRSample when it comes to AutoFDO, perhaps this's good for readability. I guess your suggestion is for the performance since we won't have virtual call. That's good. If so and we don't have other kinds of perf sample, we might don't need the base class PerfSample and can just name HybridSample to more general name(like PerfSample).

Thanks for the refactoring and clean up, looks great!

To reduce the profile loading time during postlink phase when ThinLTO is enabled, we are going to split profile into two parts. One part contains profiles with inline instance and another part contains flattened profiles without inline instance in them.

Thanks for quick fix!

In D92621#2437658, @wmi wrote:

In D92621#2435924, @wenlei wrote:

We find that after this change, the elapsed time to build a large application distributively is reduced by 5% and the accumulative cpu time used for building is also reduced by 5%.

This is great, thanks for the patch! Just to clarify, did you mean 5% of end to end ThinLTO+AutoFDO build? I'm curious what is the total profile reading time your saw in terms of %? and how different that % is between md5 profile vs non-md5 profile? Currently we mostly use non-md5 profile, assuming md5 profile is more about saving profile size.

Yes, it is 5% saving of end to end ThinLTO + AutoFDO build time.

Each module compiling has different % of profile reading time. The percentage is negligible for large source module but can be significant for small source module. I tried it on a very small file.

The build time using md5 profile was 0.55 second. 
The build time using non-md5 profile was 0.75 second. 
The build time using fixlenmd5 profile was 0.15 second. 
The build time not using any profile was 0.1 second.

Because for a large project, many source files are small or medium sizes so the building time for small files matters especially for build cpu resource. It also matters for end-to-end build time but usually not as significant. For the experiment I did (a 20M binary profile), the end-to-end build time saving was about the same as the aggregate build cpu resource saving (both were 5%), but from our experience using larger profile (like 300M profile), it will affect aggregate build cpu resource more than end-to-end build time.

rebase

I believe it's not worth doing just to be able to use one constant string in one place.
If we move them into Attributes.td, we would be adding 3 new attributes to EnumAttr, just to support this, which I think is an overkill.

We find that after this change, the elapsed time to build a large application distributively is reduced by 5% and the accumulative cpu time used for building is also reduced by 5%.

This looks great. Thanks for working on this and making all the changes!

rebase

Address Wei's feedback.

LGTM.

Thanks for reviewing and the feedbacks. I think I've addressed all of the current ones. Please take a look, thanks!

We record function entry count and branch weights and use them to compute the count when needed. This mechanism works well in a perfect world, but often breaks in real programs

This change covers non-LTO cases. For LTO, I think we would need to pass it from driver to LTO. Something like this: tools::addLTOOptions -> lld -> lto::Config (Config->TargetOptions->MCTargetOptions) ->LTO Backend.

In D87011#2379609, @dblaikie wrote:

In D87011#2379603, @ayermolo wrote:

In D87011#2378533, @ikudrin wrote:

In D87011#2378347, @jhenderson wrote:

At least in LLD, it's not quite as simple as being added after the user's code: if a library appears on the link line it will be included in the output order as soon as it is determined it is needed. Thus if you have have three modules 1.o, 2.o, and 3.o, with 3.o in an archive 3.a and 1.o requiring 3.o, you end up with an output order of 1.o 3.o 2.o if the input order was 1.o 3.a 2.o or 3.a 1.o 2.o or an output order of 1.o 2.o 3.o if the input order was 1.o 2.o 3.a. In fact, with use of the --undefined linker switch, you can even force 3.o to appear first.

I accept using --undefined or rearranging the command-line order is less than ideal, but I'm really not convinced LLD should have any place in parsing the DWARF to determine output order. Furthermore, it's not even a reliable solution - if the objects built with DWARF32 (potentially all of which might have come from libraries) are large enough, no amount of reordering will fix the behaviour. I think users who need DWARF64 in their libraries are just going to have to request DWARF64 versions of the libraries, if the --undefined and reordering command line options are insufficient.

I'd guess that for a large-scale project the recommendation to use -u would be unrealistic. We are talking about projects where debugging information in a single section can easily go beyond the 4GiB limit; it is impossible for the developer to adjust the command line manually.

By the way, from a semantic point of view, I don't think it matters if the DWARF is in a different order to the data it represents - I'm just concerned about the maintenance and performance burden of having to parse the DWARF to achieve this reordering.

There is no need to parse the debug info sections. Reading only the first 4 bytes of .debug_info is enough to assess the format (there might be input files with format intermixing, but we can ignore them in the sack of simplicity). And we do not need any automatic sorting if the size of an output section is less than 4GiB.

Exactly. Not to mention, I think for users that actually worry about 4Gig limit they have pretty complex build system that will need to be modified to get build order right. Probably doable, but looking at overall compilation pipeline, is it really the best approach? Within lld we don't have to parse entire debug section, just read few bytes in each CU to determine if it's 32 or 64 bit.
Yes theoretically it is possible that there are just so many third party libraries that they will over flow 4gig by themselves, but I think common case is they will be under 4 gigs.

FWIW, this is probably a big enough discussion to deserve it's own review, probably even it's own llvm-dev thread. My personal take would be: Unless there's a specific user who needs this, probably not worth building it. If you personally have a need or support users who need it, that swings the discussion a fair bit into "what's the best way we can help these users".

If CSSPGO has the priority based inliner, does it mean it will do most inliner work and CGSCC inliner will mostly be used as an iterative clean up pass?

In D90125#2365948, @wmi wrote:

Another question. Have you ever evaluated the performance by comparing this patch "CSSPGO profile working with existing AFDO pipeline" with the default SPGO? I understand CSSPGO's benefit has not been used without the change of CGSCC inliner. The intention of the comparison is to understand how well existing AFDO pipeline work with CSSPGO profile. It may expose problem in existing SPGO profile or CSSPGO.

Fix typo and linter, add getCanonicalFnName.

Thanks for quick review! I've update the patch, also see replies inline.

address feedback from Wei and David

If it's not too much work, I'd suggest squash the refactoring of your later patch (RVA->Offset etc from https://reviews.llvm.org/D89715) into this one. Upstream patch does not have to map 1:1 to internal patch - squashing, minimizing code churn and grouping by functionality helps make it easier for other reviewers. Thanks.

format, separate change in ControlHeightReduction, remove remaining internal markers.

Thanks for tracking down the assertion. The context profile adjustment and merging for AutoFDO today is a best effort approximation. Given the case we ran into, which is hard to handle perfectly and also very rare, removing the assertion makes sense to me.

LGTM. Thanks!

Thanks for the context! So the intention is to use subclass to hide extra complexity of uncommon sections/variants and its reader/writer into subclass. Then this refactoring makes sense.

Thanks for the refactoring. I'm wondering what is most flexible way for us to take advantage of extended binary format. I thought being able to mix and use different section is really flexible, but if we tie non-common section to specific new format/sub class, it may restrict the way we use this format. E.g. if ExtFormatA has SecProfileSymbolList, and ExtFormatB has something like SecFuncMetadata (which is what have for CSSPGO internally), then consider if there's a need for a profile to have both SecProfileSymbolList and SecFuncMetadata - implementing that through inheritance could be a bit cumbersome.

Would be good to share benchmark/performance numbers you have for this change too. Thanks.

LGTM. I think we can get this in first to address the fall out from https://reviews.llvm.org/D87470, while investigating ASAN failure.

Landed the change on behalf of @wlei.

LGTM except a minor comment.

LGTM. A small nit: the code snippet in the description could be removed as it's indeed in the codebase already. :)

Just saw this. Thanks @Alina Sbirlea for fixing the tests, much appreciated!

Committed on behalf of @ayermolo.

Committed on behalf of @modimo.