@vsk If you're curious about how the max-params value affects code size, I think I can find some time to collect some numbers soon (on firefox). I'll post it here as soon as I have it.

@vsk Hope these following data collected from compiling Firefox helps. The "Delta" values come from my previous patch: https://reviews.llvm.org/D84468. Based on the data, this patch performs very well at reducing the code size.
-Os:

Add test case for EH outlining

Update comments.

This is marked WIP since it might be better to do the code transformation inside CodeExtractor instead of HotColdSplitting. With HCS, we're currently doing it before code extraction, but the region might end up being unworthy for extraction. But if we do it in CodeExtractor, we'll need another implementation of OutliningRegion to detect the SESE region enclosing the EH blocks.

In D85628#2214429, @yfeldblum wrote:

In D85628#2214314, @hiraditya wrote:

If there was a way to provide handwritten profile/coverage file, maybe that would work in absence of profile information?

I am not sure I see a need for profiles here.

I don't think we need profile information. We just need __cxa_guard_acquire to be marked cold, and for the compiler to infer coldness of code in the same block as a call to something marked as cold. (Apparently HCS does this?)

Update test case to reflect newest changes

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.

Mark cold functions in advance before outlining them.

Add the test command for read from file.

Add a test case for command line-supplied cold functions list.

Remove unused includes.

Support marking declarations, e.g. in @hiraditya's example of __cxa_guard_acquire/abort, as cold.

Add debug messages.

Address review comments from @vsk, @hiraditya

Based on discussion with @hiraditya and @rcorcs this morning, we're thinking doing this in in the ProfileSummaryInfo pass is better (have the PSI pass take as input user-specified cold funcs and mark them with cold attribute) as its benefit can be more wide-ranging than only for hotcoldsplit. Will prepare a separate patch for this.

@vsk Apologies for the late reply. Here is the data on:

Would also really like to see this patch landed. Also, as of today (8/10/2020), the patch cannot be cleanly applied into trunk without inducing merge conflicts anymore.

The tests are modified from split-cold-2.ll in the same folder. Add comments to note this.

@rcorcs Your comments are addressed. Thanks!

Incorporate @rcorcs 's suggestions.

Add tests

Will add test case soon.

Accepted based on discussion with @hiraditya

Looks like coldentrycount.ll is still causing a problem @hiraditya .

Seems like this is caused by target datalayout and target triple. See https://reviews.llvm.org/D85215 for proposed fix.

@JamesNagurne We proposed a fix in https://reviews.llvm.org/D85215, instead of moving it into X86/, we made the test cases architecture-agnostic.

In D69257#2190753, @CarolineConcatto wrote:

@hiraditya
We believe that this patch has caused the Flang buildbots to fail:
http://lab.llvm.org:8011/builders/flang-aarch64-ubuntu/builds/2437
http://lab.llvm.org:8011/builders/flang-aarch64-ubuntu-clang/builds/579
It would be nice if someone could take and action to revert of fix it ASAP.
Otherwise we cannot verify the other patches properly.

Update diff to fix merge conflicts.

Add testcases for c stdlib longjmp/setjmp functions in addition to builtin_longjmp and builtin_setjmp.

equals should be contains

@lebedev.ri Just a suggestion, but maybe the title should now be "[HotColdSplit] Add test case for unlikely attribute in outlined function"? Since this patch is now only adding a test case.

Check if function name equals longjmp when checking for blocks
including calls to longjmp; do not check for prefixes and suffixes
anymore, as it might cause potential problems in the future.

@hiraditya The testcase is passing. This should be ready to be merged.

Squash commits into one.

Update to include testcase with profile information, and check for "longjmp" suffix when detecting blocks containing a longjmp (since the intrinsic name is now "eh_sjlj_longjmp").

Add testcase for longjmp.

ping @hiraditya .

Add testcase for outlining cold block containing assert fail.

@vsk I'll post some relevant statistics on the distribution of benefit-penalty scores on our workloads first:

Change default SplittingDelta to zero.

Based on discussion with @hiraditya today we'll change the default to 0 (which stays the same) and add additional test cases for delta=5 (which come from the four failing cases here).

Ping for review.

Resolved merge conflicts.

Seems like the patch isn't applying cleanly (which is causing the build failure). I'm looking into it.

Commandeering this revision as discussed with @hiraditya .

Utilize profile information to mark longjmp blocks as unlikely executed if they are cold.

Remove use of CallSite and update longjmp intrinsics (since Intrinsics::longjmp is removed, changed to using Intrinsics::eh_sjlj_longjmp).

Merges D52896 with trunk