LGTM, if there are outstanding comments from other reviewers we can address them in subsequent patches.

In D35035#1284164, @jfb wrote:

In D35035#1282236, @spatel wrote:

In D35035#1281462, @lebedev.ri wrote:

Not sure what is the general consensus wrt this patch, but i guess it now consistently uses bytes.

Agree - the code change looks like what I expected now.

So:

1. Is @jfb objecting to this as an intermediate improvement?

My concern is here: https://reviews.llvm.org/D35035#inline-464768
This uses an unrelated constant to drive optimization decisions. Create a new per-target constant.

Thanks for removing the exception handlers. I never wanted to hard code them in the first place.

Can we push this patch? This is not enabled by default so we can continue development in subsequent patches.

Comment

Remove ModuleSummaryIndex

For reference: https://reviews.llvm.org/D53887

For splitting more than one cold region, maintaining a DT maybe expensive but we don't have to do that. All we need is to 'color/mark' the blocks which we want to outline. In the next iteration the colored blocks need not be considered. It may be slightly non-trivial in a general case where coloring SEME and reasoning about DT/PDT of blocks which are non-colored. What we can do is in the subsequent iterations we can take a sub-graph which do not intersect anywhere except at entry or exit. This way DT/PDT will still be preserved in the non-intersecting regions. I think jump-threading also works(should work) on similar lines.

Adding test

Fix testcases and address comments

Can we land this patch

Any updates?

In D52904#1265895, @sebpop wrote:

In D52904#1265893, @hiraditya wrote:

With this patch I now see a 3% speedup on sqlite vs. no hot-cold-split pass.

Awesome! Thanks for improving this!

I think the 3% uplift in performance is within the noise level.
At least with this patch I do not see the 10% regression in performance with hot/cold partitioning
which most likely was due to doubling the cost of function calls when the whole function was outlined.

With this patch I now see a 3% speedup on sqlite vs. no hot-cold-split pass.

Awesome! Thanks for improving this!

llvm_debug

Compiler error with CloneType

compiler error

DEBUG -> LLVM_DEBUG

In D35035#1252975, @spatel wrote:

In D35035#1252829, @hiraditya wrote:

In D35035#1252796, @SjoerdMeijer wrote:

Perhaps the impact is negligible, non-existent, and we worry about this for nothing. As also suggested earlier, I will try to get some numbers on the table for ARM and AArch64 if we strip out the lowering here, if that is helpful for this discussion, but probably need a day or two to get them.

If you could provide some numbers, I can go ahead and remove the inlining of memcpy altogether provided the reviewers agree with it, or we can merge this patch which is trying to improve on previously hardcoded numbers.

Yes, I support removing the expansion entirely, but I don't think we can commit that change without doing some advance perf testing.

In D35035#1252796, @SjoerdMeijer wrote:

The ultimate goal would be to simply always canonicalize to memcpy and not expand it ever in instcombine as mentioned in D52081.

Looks like we all agree on this now.

But we don't do that (yet) because we're afraid of missed optimizations that can't be replicated in the backend.

Perhaps the impact is negligible, non-existent, and we worry about this for nothing. As also suggested earlier, I will try to get some numbers on the table for ARM and AArch64 if we strip out the lowering here, if that is helpful for this discussion, but probably need a day or two to get them.

Modified the testcases to use both pass managers
Use single commandline flag for both pass managers.

Updated testcases with utils/update_test_checks.py
use isPowerOf2_64

Added support for new PM: https://reviews.llvm.org/D52708

Will add one soon!

Moved the check of Size before checking the LargestInt.

Fixed all testcases, and removed memcpy inlining when target datalayout is not present.

Fix some unit-tests, a few more remaining.

To fix tests and comments from reviewers.

My main worry is that this is only useful with LTO because the linker should do this work otherwise, so at a minimum I need to change the code accordingly.

In D35035#1238511, @lebedev.ri wrote:

In D35035#1238502, @hiraditya wrote:

In D35035#1237497, @spatel wrote:

In D35035#1237401, @hiraditya wrote:

I think this patch is good to go, I can push this if someone accepts. I'll fix the comments.

I still don't understand the (LargestInt == 0) hack. I think everyone agrees that the existing code is wrong, so why preserve the existing behavior if we don't have a valid datalayout? Ie, there's no risk for real targets because they always have a non-zero getLargestLegalIntTypeSizeInBits()?

Agreed, I'll remove that part then. That was only to make other testcases happy by preserving existing behavior. Thanks

Presumably those testcases should simply be updated to contain the expected datalayout?

In D35035#1237497, @spatel wrote:

In D35035#1237401, @hiraditya wrote:

I think this patch is good to go, I can push this if someone accepts. I'll fix the comments.

I still don't understand the (LargestInt == 0) hack. I think everyone agrees that the existing code is wrong, so why preserve the existing behavior if we don't have a valid datalayout? Ie, there's no risk for real targets because they always have a non-zero getLargestLegalIntTypeSizeInBits()?

I think this patch is good to go, I can push this if someone accepts. I'll fix the comments.

If you have the code-size numbers, please provide that then I can accept the patch.

If there are no-more suggestions for this first draft I'd like to get this in (disabled by default) such that people can try and give feedback. Thanks!

Rebase onto top of trunk

I'll rebase onto top of trunk.

Check calling convention of outlined function to discard it form hot-cold split optimization

Added testcase and disable recursive outlining of cold-functions.

Fixed copy-paste error.

Suggestions from Sebastian:

Forward propagate profile information to all cold-blocks.

Do you need help with pushing the changes?