Handle pre-increment case as well, improve comment, rebase over additional tests.

LG

Just to be clear, I didn't mean to say that we shouldn't do this, just that it is not guaranteed to be a win in the end. If it's possible to do some end-to-end performance testing with this patch to make sure that it doesn't come with unexpected regressions, I see no reason not to move forward with this change. The problem with assumes is a long-standing one and I don't think it has any simple solution.

LGTM. In the future, it might be nice to generalize this as a getConstFloatRange() function (that would be [-INF, C] for minnum and [C, INF] for maxnum) and fold the comparison based on that.

In D60935#1502223, @reames wrote:

Reading through the discussion, it sounds like the only place where LFTR goes wrong is dealing with the post-increment form of the IV right?

If so, what about splitting LFTR into two parts. The first part does LFTR *without changing pre-to-post or vice versa*, and then a second part which converts to post-increment where possible. The value of the later seems quite questionable to me honestly, and if we have to restrict it greatly it wouldn't matter as much as having to restrict the first part. I'd also really like to be able to extend the first part to multiple exit loops whereas extending the second part seems actively harmful on some examples.

A few last issues...

Add -asm-verbose=false, move CHECK lines, remove tabs.

This one can probably be abandoned in favor of D60261?

Reverse ping. Apart from the previous comment this looks good, and I saw you already added some tests for that as well :)

Add the last missing binary op to ConstantRange: sdiv.

Rereading the ML discussion just now, it looks like the consensus is to go with this option. Move forward?

This looks good, but please also update existing tests that specify three types in the intrinstic name. Here's ones I found:

Rebase over D61744.

I'm going to drop this for now -- the original motivation here went away with the signed intersection support. These changes still make sense to improve icmp instsimplify, but are also not super valuable in that instcombine can handle it.

Still needs some duplicates CHECKs removed, per comments above.

@spatel What do you think about going for the following abstraction instead? What I have in mind here is to make this also reusable for operations other than binaryOp(), such as uadd_sat() and friends.

Here's a slightly cleaned up version of the reproducer:

Vector op legalization will automatically recursively legalize the returned SDValue, but we need to take care of the other results ourselves.

@uabelho Thanks for the reproducer. I think the problems is that when we thread a comparison over a phi in instsimplify, we retain the original context instruction, while (I think) we should be using the terminator of the incoming value as context.

@aemerson Thanks for the report! From a quick look, looks like the usubsat expands to usubo, which is then expanded during op legalization, while it needs to be expanded during vector op legalization. We're doing that for umulo and smulo, but apparently not for uaddo/saddo and usubo/ssubo. I'll prepare a patch.

I've reverted this change for now (rL360260).

Abandoning this one, don't think there's anything worthwhile left here.

Use both pre-inc and post-inc AddRec nowrap flags.

LGTM

In D61409#1487372, @lebedev.ri wrote:

In D61409#1487366, @nikic wrote:

This is a good idea in theory, but I'm somewhat concerned that this will backfire in practice because assumes often pessimize optimization. For example LLVM has a really bad offender where it inserts assumes for alignment annotations during inlining -- thankfully there's an option to disable it, because it's a major perf regression otherwise.

Pessimization of the final produced code, or of the llvm itself?

This is a good idea in theory, but I'm somewhat concerned that this will backfire in practice because assumes often pessimize optimization. For example LLVM has a really bad offender where it inserts assumes for alignment annotations during inlining -- thankfully there's an option to disable it, because it's a major perf regression otherwise.

I suspect that it's not safe to do this in this form: While the shift instruction on the target might be well defined, this is still operating on ISD opcodes that consider out-of-bounds shifts to be undef. After you have eliminated the check, some other combine could come along and optimize the shift to undef based on that.

ping

In D60846#1481903, @dlrobertson wrote:

In D60846#1481782, @spatel wrote:

In D60846#1481774, @nikic wrote:

test/Transforms/LICM/hoist-mustexec.ll still needs to be updated. (Should probably generate full checks as a preliminary NFC commit first.)

Oops - I forgot that even though I mentioned it in the earlier comment. Yes, that must be included in this patch (and any other existing tests that are known to be affected).

Sorry, I forgot all about this as well. I did run the update_test_checks script on this file after applying the patch and there was not improvement to the generated code.

Transforms/LICM/hoist-mustexec.ll still needs to be updated.

Can you please point to patch 5? I think it's missing. (Best directly add it as a parent revision.)

As far as I know we don't have any known issues in funnel shift optimization or codegen. We should check with @spatel though.