Ping.

I've reverted the original problematic change by rL336410. I think we need to collect tests that degrade from it to make this change properly.

I failed to construct pure SCEV test easily, will try unit test if needed.

In D48930#1152458, @spatel wrote:

In D48930#1152437, @mkazantsev wrote:

The context is following: the patch rL336172 has broken this test, and I was asked to fix it.

I assumed that much. :)
But are there any other examples that were given to you? The solution may depend on how varied the breakage is. If it's just this case, then we can probably get by with a solution that doesn't need value tracking.

The context is following: the patch rL336172 has broken this test, and I was asked to fix it. Basically I just restored the behavior that we used to have before this sinking. I would prefer to not introduce any new transforms to InstCombine without dire need because it is fragile, and I am not familiar with it well enough (in particular, I don't understand ordering of particular transforms in many places). So I've chosen a conservative approach (to restore old behavior).

Rebased on top of pre-merged test, fixed comments.

Added some comments, removed one unnecessary lambda.

LGTM

LGTM once a TODO comment is added.

Frankly, I don't understand why we want it to be an instruction at all. Can these methods (i.e. hoistIVInc and isNormalAddRecExprPHI) be taught to work with values? For example, if it is not an instruction then we just consider it already hoisted.

Moved fill to private API, added lazy calculations, overall it is now more user-friendly.

Rebased. We need to reanimate this.

Sinking it to the end of method did not introduce any new changes. Also added a comment.

In D48584#1143240, @lebedev.ri wrote:

This only moves the foldICmpUsingKnownBits() call later on,
but in PR37636 @spatel suggests it *might* expose more problems.
Does this expose any problems?

In D48584#1143240, @lebedev.ri wrote:

This only moves the foldICmpUsingKnownBits() call later on,
but in PR37636 @spatel suggests it *might* expose more problems.
Does this expose any problems?

LGTM

Thanks for fixing that!

LGTM

Not sure how to construct a test that will fail an assert, however the general idea is that, for example, a block that goes to method exit from the innermost look is also an exiting block for all its containing loops, up to the topmost parent. It means that this exiting block may be considered for iter count calculation for all those loops. From this point of view, there is no practical difference between the immediate parent and any other top loop.

I will do it as a follow-up then.

I'm not sure if it gives benefits, but the idea is interesting. Do you mind if it goes as a follow-up?

Ping?

LGTM once comments are addressed.

Never mind, in this case the latch of outer loop changes, so we must clear cache.

Actually particularly with case of unswitching, I grow hesitant about one of our asserts that fails here. Maybe it is over-conservative. See comment https://reviews.llvm.org/D44676#1086144

This one appeared to be a real bug, at least we have a test that fails with assert for trivial unswitching. Thanks @uabelho for providing the test case.

I am not sure that we actually break the invariant which was preserved before. Note that all recent failures we have seen were on assert from the patch D44676. We might as well just break this invariant previously, but without this assert we did not know about it.