Yeah, this limitation seems arbitrary and overly conservative.

Can we handle the case with inbounds offsets only first and extend with non-inbounds support in a follow up change? This way in the first change you'll not need to change IR interfaces at all.

LGTM. Maybe add a test when the WC has one use but the branch condition is used in multiple branches.

Ping?

This change was resubmitted as D68006.

In D67870#1680256, @efriedma wrote:

This is specifically a hole in the calloc() handling, right? This case can't come up in regular load->store no-op stores: the address of the load dominates the load, so the address dominates all the operations between the load and the store, so alias() does the right thing.

That's right. I don't think that load->store case is affected.

It looks like you successfully avoid the pitfall the original original loop predication fell into by filtering implicit exits and using trip counts. It makes me think that we can revisit loop predication again and go back to SCEV based implementation.

+1 to a test.

In D67870#1680256, @efriedma wrote:

It looks like you forgot to CC llvm-commits? Please post a new differential so the patch is sent to the list.

Resubmitted as D68006.

We should just skip debug instructions while looking for the guard in the window. Just use getNextNonDebugInstruction instead of current getNextNode.

The original patch introduces a function minIterationWidthForEvaluateAtIteration. This way it exposes the limitation of evaluateAtIteration through the API and makes it possible for the user to choose the proper It width (e.g. like I did in the first revision https://reviews.llvm.org/D62563?vs=on&id=201795). What was the reason to remove it?

Even though it's motivated by downstream code, looks harmless to change upstream.

Fall back to non-canonical mode for non-affine addrecs.

LGTM

In D62563#1521477, @reames wrote:

Artur, nice find. In terms of staging complexity, have you considered how impactful it would be to simply refuse to generate the value at the given iteration in this case? evaluateAtIteration is allowed to return SCEVCouldNotCompute. I'm tempted to first introduce a bailout for a quick correctness fix - maybe along side your assert to see if we're missing any other cases - and then spend more time considering your full fix.

Have you considered reusing calculateByteProvider mechanism from load combining instead of matching an exact pattern?

LGTM. I agree that we can leave the stronger variant for the future.

Looks good overall, a couple of comments inline.

1. Since we need to fiddle with the insertion point I think we should stop using single IRBuilder which we pass around. It looks like creating a local IRBuilder is a cheap operation, so we can have local IRBuilders when we need to generate instructions. This way the insertion point will be clear from the local context.

Add diff with context.

One of the alternatives naming schemes which was discussed is to call this intrinsic should_deoptimize. In this case the meaning of the returned value is inverted, so we need to or it with the condition which we want to widen.

With this change you seem to be invalidating AST for instructions which have not been previously invalidated. I would hesitate to call this NFC.

Current patch seems to have comments like "this block doesn't need invalidation". Maybe sink this decision down to the SafetyInfo implementation? It should reduce the complexity of the user: just call invalidate every time you delete/insert instructions and you would be fine.

@rcorcs this has been accepted for quite a while, is there anything preventing integration? I'm looking forward to using colored graphs out of the box.

Can you describe what is the difference you observe in the verifier behavior with and without simplifications before? There will always be cases when some simplifications before the verifier change the CFG. You won't be able to handle everything in this analysis.

The suggestion to apply calcInvokeHeuristics before calcUnreachableHeuristics and calcColdCallHeuristics makes sense.

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.