The MBFIWrapper change seems NFC, can it be extracted out first?

ok with me if Reid is ok with windows specific logic.

Can you extract Window's specific code into its own helper function if possible?

lgtm

there are checks of explicit occurrences of inline-threshold option later, so the behavior seems unchanged.

ok. I see the intention. As long as the behavior of --inline-threshold option is not changed (it still overrides the new option), the new option seems fine to me. Adding individually controlled option for size opt seems a good idea too.

This change won't work. See

lgtm

lgtm

As other commented, please extract the code into its own function also add the support when AA is available ( as other parts of the function does).

lgtm

lgtm

thanks for the cleanup. The implicit conversion was not quite readable. LGTM

lgtm

InlineResult --> inlining related result (viability, etc) -- it captures two pieces of information: 1) inline decision and 2) when decision is 'no', related inline analysis that leads to the no decision. The class name seems fine. The patch makes the 'decision' part more explicit, and also fixes some bug in missing the right analysis message.

Class Name ResultWithMessage sounds too generic. Why not keeping the InlineResult class name? The rest of the changes look reasonable.

lgtm

lgtm

lgtm

The intention is to make the base CallAnalysis becomes a symbolic execution engine (virtual optimizations) what can be reused. The cost tracking is extracted into the derived class.

This looks useful. Is it possible to add a test case?

lgtm

this looks good to me. Easwaran, do you have any comments on the refactoring?

This patch will be part of mtrofin's https://reviews.llvm.org/D71733, so there is no need for it.

Use AAManager (include/llvm/Analysis/AliasAnalysis.h) by registering only BasicAA ?

The SROA handling code should also be abstracted away and let the derived class handling cost accumulation. In particular, the common code pattern like:

BasicAA is stateless and should be available and used here to disambiguate.

lgtm

There is comment like this:

lgtm

lgtm

lgtm

lgtm

lgtm

lgtm

lgtm

lgtm

what I mentioned should be complementary to the top-down method in this patch -- it just allows the full top-down to be doable for cross module scenario as well.

One way to handle it is 1) delay early inlining of sites into a hot function if a big percentage of calls to the function are from other modules. This still allows intra module top down inlining of it; or 2) keep a clone of the unlined body of the original function and use that one during cross module inlining.

This looks good. Can this be handled for cross module (thinLTO) case somehow too?

lgtm

How is the query type going to be checked?

Can this be derived statically from the FuncT or BlockT?

lgtm

Perhaps just add additional checks on the content of the dumped profile to make the test case more complete.

thanks for the test case.

lgtm

Since the unlock is the common path, why not replacing goto with a helper as well?

Is it possible to add a test?

Around line 1323 of InlineCost.cpp, there is code that detects if the new indirect callsites can become direct sites after inlining. This information can be passed back to the caller (via GetInlineCost and shouldInline interfaces). WIth that, I think the compile time impact can be minimized.

rebased and addressed review feedbacks.

The compile time impact looks too high. Might consider more fine grained check to trigger more iterations.

lgtm

Any compile time impact numbers?

/* */ is more common, so perhaps make it consistent to this. lgtm otherwise

Looks good to me. Added Sanjay and Simon for review.

May I suggest another split? DAG.shouldOptForSize change can be split out as a NFC.

If the runtime/size overhead is acceptable, I prefer this simpler version of implementation. We can always document the improvement possibility in the source and enhance it later if needed.

lgtm

In this case (shown in godbot), why does PM decides to not iterate another round?

lgtm.

Ping.

With SizeOpts change split, is it ready to split TargetLowering changes out?

lgtm

Thanks. lgtm

If we can clean up the code, that will be great.

maybe we should make lprofCurFilename hidden?

I see.

shared lib should have different profile name from the main executable so the test should check that two file names are not identical?

lgtm

Handling what Wei's case will be a nice thing to have, but it may require more significant change in JT. Currently the JT candidate BB selection is based on checking the conditional value used by branch or return value of ret instr (with this patch).

Wenlei, this sounds like a good idea. Patches are welcome!

JumpThreading is basically basic block cloning followed by control flow simplification. This is just a special case where the second part is missing.