lgtm

Looks like div by zero error.

ThinLTO is known to have issues related to profile update (cross module), so we were thinking something similar in ThinLink phase. One of the issues is that the pre-inlining needs to make similar decisions as the compiler. How well is the preinliner doing in this regard?

lgtm

Regarding the staging of the patch, here is the suggestion:

lgtm

lgtm

that is fine if it does not add visible compile time overhead (e.g, to
compute and track size).

Hiroshi is working on the implementation, and I expect upstreaming happening soon. In a nutshell, the working set analysis is based on building interprocedural loop graph (ILG) and working set propagation.

Having cap on the target context (where a site is inlined into) is useful for performance purpose but it is based on working set analysis which we are working on. This can be 'overloaded' to control compile time growth too.

Without guarding the caller size, this (over topdown inlining) can also potentially happen without recursion.

Right . We (snehasishk) have plan to tune MFS for AutoFDO. Once that is done, the inline cost analysis for AFDO can be revisited.

It is probably related to MFS -- machine function splitting is not yet well tuned for AutoFDO, and the cost/benefit analysis needs to sync up with that to that for estimating cold size properlly.

lgtm

lgtm

thanks for the background. This patch looks good at higher level. Vedant can help detailed review.

Is the case when there is no counters very rare? And for those cases, how much overhead the runtime hook can incur? I assume it is small compared with actual instrumentation?

LGTM (the a.out file is probably accidentally added).

lgtm

Perhaps add a test case showing that up to 3 ranges can be promoted?

lgtm

not sure if one is used by downstream users -- on the other hand if is used, the name of the API should be changed.

lgtm for ELF. Anything to do for COFF?

So to summarize, this patch is to enable compiler passes to respect comdat semantics while still enabling linker to do GC (after using GNU_RETAIN)?

The logic seems complicated. Is it better to use (uint64)-1 as the special value to mark promoted candidates?
( I posted this comment on the wrong thread).

The logic seems complicated. Is it better to use (uint64)-1 as the special value to mark promoted candidates?

lgtm

Kazu has seen this error

lgtm

lgtm

So in this case, the virtual call in user code is resolved to a strong definition in the shared library even though user code also has a derived class defined? In other words, the library provides another overrider definition as the final overrider?

llvm_prf_names are cross function for the purpose of compression. This section is not loadable, so it is does not really matter that much.

lgtm

Does it work for other linkers with this change?

lgtm

thanks for the update. lgtm

Nice reduction. Is the total .o size quoted from instrumentation build (with --gc-sections)?

Good point about the section header. The objective of the patch is to reduce binary image size (loadable sections) though the object size may increase. It would be good to know the object size overhead because some of our large binaries may get hit during link time (of instr binary).

Looks good to me. Wait for maskray's comments.

Can this be handled by other linkers (gold, bfd)?

lgtm

I think this is probably worth a user level option as well.

lgtm. Please wait for other reviewer's feedback too.

no it is not noise -- the questions you raised are very good :)

I think what Rong means is that

1. moving code to a header is for code sharing between IR and codeGenPass
2. Wei's idea of avoiding the header move can lead to circular dependencies.

Good to know that SampleCoverageTracker can remain as it is (not templatized). Is it possible to not move it into the header? There are a few advantages:

1. reduce the size of the patch and merge churns;
2. a little better in compile time as the functions don't need to be built when building FlowSensitiveSampleLoader.

question: it seems SampleCoverageTracker implementation is independent of IR/MIR level. Do we need to templatize it ?

Is the intention to move them (inline bodies) into the header? For those functions that are not, leave them as being defined out of line. Otherwise moving them into class body is a preparation for the next step (moving to header).

Instead of having inlining keyword, it is better to move the out of line definitions as inline bodies in the class.

Why making callSiteIsHot a file static func?

This will make downstream integration/merging and the review process easier.

The diff is quite large and it is better to be broken down into smaller pieces in stages.

lgtm

my bad -- I missed the test case. LGTM

lgtm

Should it be warning or error?

Do you have an example showing the non-determinsticness ?

I agree with Rong here -- the semantic related error checking belongs to the consumer/client, not in the tool.

lgtm

Should we get rid of DefaultInlineAdvice and fold it into the base class InlineAdvice? 'DefaultInlineAdvice' implies it is an advice provided by the default advisor which no longer is true.

You are right -- the text format is properly implemented.

The proposal in the patch is to use line:col.discriminator (which is reasonable), but the implementation uses line:col:discriminator -- please update the patch to be consistent.

lgtm

A few high level questions:

lgtm

lgtm

I suggest also dumping some opt remarks under --pass-remarks-analysis. This also allows you to write test cases to cover various types of savings.

thanks for working on the test -- good to add later if possible. LGTM after addressing snehasish's comment.

lgtm