Any plans to merge this patch?

nit: If FIXME's are mostly future works, then please replace them with TODOs.

Is the test failure related to this patch?

Self approving because this is NFC change to outline function definitions.

rebase

then it may be possible to construct a synthetic/fake profile as a pre-processing step

Rebase

Addressed fhahn's comments

please run clang-format

49a944af7f1980b54d8a2be0ef640f5a956bc423

FWIW C++ doesn't really have a lattice anymore: https://wg21.link/p0418

Is there a guidance for the users of 'isStrongerThan', isStrongerThanUnordered etc.?

Is this patch still useful now that D53514 has landed?

please run clang-format.

..anyway I can try to describe how thing work using some example. Is this what you are looking for?

yes. Thank you.

Can we add more test cases to include eh_pad, invokeinst,

please run clang-format on the patch.

Overall approach looks good to me even when we don't see good SPEC-17 numbers as the optimization is intended to reduce page faults. The improvements would be more pronounced in large applications. I'll review the code in more detail in the next few days. Thanks for working on this.

Is this work based on any paper/implementation?
Can we add some documentation at the top of the file to get an overall idea of the cost model?

Fixed the bug in: https://bugs.llvm.org/show_bug.cgi?id=45442

Updated test cases.

FYI: libstdc++ patch sent for review: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551934.html

Self approving as it is a trivial change and fixes PR45442

In D85628#2214119, @jfb wrote:

In D85628#2213940, @rjf wrote:

In D85628#2213919, @vsk wrote:

I’m not convinced this is a good idea. In what use case is it not possible to mark up relevant functions? It doesn’t make sense to me to make alternations to standard library functions within the compiler. It seems better to simply patch the standard library. In some cases llvm does infer function attributes for library functions, but these are generally lower level attributes that can’t be specified at the source level, and the attribute is made available to other passes in the pipeline.

Do you mean this patch isn't a good idea in general, or the recent revision isn't a good idea? For the latter, I'm not sure if you meant we should not outline declarations or we should not split the original loop into two (e.g. marking as cold before outlining). IMO splitting the loop into two simply addresses what the original intent of what we're doing, which is to mark certain functions as cold before outlining. Whereas, if we don't outline declarations via user-provided input, it renders @hiraditya 's proposed testcase useless. Alternatively, we don't have to make the testcase involving standard library functions if that's what you want :).

My understanding is that today code can be considered "cold" based on the following:

1. Attribute on the function
2. Likely / unlikely annotations
3. Profile information
4. Other compiler heuristics

This adds another way to do it, but it's kind of a side-injection and it doesn't seem particularly principled. Presumably the list you're feeding through the command-line comes from a profile? Why isn't it provided as profile information?

The patch looks legit. Wonder if such a patch should be sent to libstdc++ too.

https://bugs.llvm.org/show_bug.cgi?id=42283 has been fixed.

Closed in:
f902a7eccf30a762d9a533c3884edfb692c26a09

In D85628#2214751, @wenlei wrote:

For my 2nd question, this patch is what I think how this should be done: https://reviews.llvm.org/D79485

In D85628#2214614, @yfeldblum wrote:

In D85628#2214536, @rjf wrote:

I think you missed Aditya's point here: he's saying that certain things like outlining __cxa* functions might be unwise to mark as cold in general

Maybe I missed that, or maybe I am missing some context. I didn't think there was a top-line goal of injecting cold attributes to arbitrary functions.

My understanding is that a key motivating case is minimizing native codegen for local static variables by understanding that the slow path is definitionally cold. If coldness propagates, then as long as __cxa_guard_acquire is attributed as cold, coldness propagation should extend to everywhere of interest. And since __cxa_guard_acquire is used for only one purpose, namely guarding local static variable initialization, and since that purpose is definitionally cold, it should be attributed as cold. I didn't think there was a need to inject cold attributes to other functions.

When it comes to library functions which are cold, presumably we can just patch libraries with cold-attributed function definitions but unattributed declarations, and attribute the declarations.

Added a patch to libcxxabi: https://reviews.llvm.org/D85873

Apparently there is already support for extended binary profile format (https://reviews.llvm.org/D66766) and we maybe able to use that. Thanks to @wenlei for sharing this information.

I don't think we need to mark the object constructor as cold. We want to mark inlined slow paths of local static variables (the call to __cxa_guard_acquire, the call to the object constructor, etc.,) as cold.

It doesn’t make sense to me to make alternations to standard library functions within the compiler. It seems better to simply patch the standard library.

In D85628#2212047, @vsk wrote:

Any reason not to mark up the relevant functions with __attribute__((cold)) directly?

Please fix clang-tidy warnings.

Thanks for adding the results, could you share the script to measure bootstrap numbers?

cc: @t.p.northover does this fix look reasonable for PR37240?

We evaluated the Machine Function Splitter pass on clang bootstrap and SPECInt 2017.

LGTM. nice find!

Please share performance numbers for publicly available workload(s).

Fixed in: D85232

Fixed in: D85232

The ARM build bots do not compile this target as part of their build, causing this test to abort when '-codegenprepare' is added without any REQUIRES clause.

reverted : D85229

Reverted my original diff; D85229

will revert the old patch.

This looks like a more reasonable fix for now.

Should I land D85148 then? I'd like to keep the test case if possible.

Fix available in: D85215

Thanks for fixing it quickly @rjf

Looking into this. Seems like removing target triple should fix it.