In D65376#1612295, @lebedev.ri wrote:

Do you actually need truly random, non-deterministic transform?

As a future direction for code reordering in llvm, a pass that operates on the machine instruction level may have more promise. At that level, there's a reduced/eliminated code size penalty and landingpads can be moved easily. There's less pass ordering flexibility, but experiments seem to show that late splitting works better anyway.

In D65376#1607799, @vsk wrote:

I think that this is probably too risky. I've seen that splitting out the wrong block can introduce major performance regressions (up to and including stack overflow).

As a future direction for code reordering in llvm, a pass that operates on the machine instruction level may have more promise. At that level, there's a reduced/eliminated code size penalty and landingpads can be moved easily. There's less pass ordering flexibility, but experiments seem to show that late splitting works better anyway.

Updated to have a fixed sequence of 'random' numbers.

In D65376#1604460, @tejohnson wrote:

In D65376#1603895, @lebedev.ri wrote:

I'm not sure it is a good idea to have non-deterministic optimizations.

I agree with @lebedev.ri, I don't think non-deterministic optimizations are good as it would make triaging and debugging really difficult. I see @pcc has a follow on suggestion to make this deterministic. At the least that would be needed. But is it useful to do this optimization randomly in practice?

We can certainly make this deterministic with a sequence of lists. Thanks for the suggestion, I'll update this soon.

Thanks for addressing the feedback @emmettneyman , LGTM, deferring to other reviewers for final call.

Working on porting this to latest llvm, will push the latest changes soon.

Interesting, I have a merge-sets implementation in my patch: https://reviews.llvm.org/D32140
It will be interesting to compare the two.

LGTM

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.