This is very similar to what we had as an initial implementation of loop predication. Later we found that this approach is problematic. See https://reviews.llvm.org/D37569 description for the details. This patch likely suffers from a similar problem.

Thanks for adding the test case!

We also observed regressions caused by this change. In our case the change increases the perceived cost of a loop which in turn reduced the unrolling factor. Let me know if you need further details about the regressions.

In D39097#905174, @anna wrote:

Artur, before this patch, we were checking whether RangeCheckIV's postIncExpr is latchCheckIV.

Now, the header comments and proof seems to point to the fact that the guard and latch are using the same offset (lines 103 - 110), but the code does not have any check that we are using the same offset. Or, is it that the offset is the same, but the difference in the latchCheckIV versus rangeCheckIV is caught in their respective start values, and the offset for both latch and guard remains the same?

I changed the loop in the description so it doesn't use Start. Instead the offsets are sunk into predicates G and B.

Clarified the description, added the requested test case.

Upload diff with context. Minor fix in the header comment.

Did you measure the compile time impact somehow?

In D38547#888359, @RKSimon wrote:

Is this PR10114 ?

@andrew.w.kaylor - ping, do you have any comments about the latest revision?

Add the comment about overly conservative legality check, add the test demonstrating it.

Update, I didn't see that there was a request to add a comment. Will do and update the patch,

Address review comments.

In D31182#706365, @spatel wrote:

Before getting to any details about the patch, we need to address the question raised in PR27036: why are we doing this transform in InstCombine at all? The assumption is that an integer add is more canonical and/or cheaper than an FP add. Is that universally true?

It's difficult to tell. Even if FP addition is cheaper the second combine also saves us one sitofp conversion. It should be also taken into account.

It's worth noting for others that the motivation for the change is a bug we found in JumpThreading LVI cache invalidation. The bug is easily triggered with our downstream changes, but is difficult to trigger it upstream. We need the printer pass to write an isolated upstream test case for this problem.

Add an assert as suggested.

• Land tests separately and rebase the patch against it.
• Remove addrspace(1) from the test cases.
• Change CHECK pattern matching in fp16-promote.ll test_extractelement slightly.

Fix the type CHECK64 -> CHECK-ARMv6 in ARM load-combine.ll test case.

The test cases have been landed separately (rL294185), rebase the patch.