[PATCH] D103656: [ScalarEvolution] Ensure backedge-taken counts are not pointers.

Wed Jun 16 08:14:27 PDT 2021

reames added a comment.

In D103656#2821141 <https://reviews.llvm.org/D103656#2821141>, @efriedma wrote:

>> Having those queries return unknown more often would be undesirable.
>
> I agree.
>
>> Second, the getLosslessPtrToIntExpr routine seems to not match what you're describing. It appears to have a bunch of cases where it does not succeed. Examples include min/max expressions, and udivs. (Going by code structure here. If I'm missing something, let me know.) Are you claiming that this routine is guaranteed to succeed? The existing code and comments don't seem to make this obvious.
>
> SCEVPtrToIntSinkingRewriter always succeeds; it inherits implementations for every kind of expression from SCEVRewriteVisitor.  The explicit implementations of visitAddExpr and visitMulExpr are just there to preserve the flags.  Not sure how you're reaching the conclusion that it fails for min/max expressions.

Reading back through, I see you are correct.  The only bailout (e.g. return of SCEVCouldNotCompute) is the effective type size check.  Can you update/add some comments to the function to that effect?

>> If you want to achieve the stated goal, have you tried checking for lossless conversion of the *result* of the exit computation? If that worked, it would be a much cleaner and more obvious change. (e.g. never return an exit count which can't be converted to integer type) This would mean instrumenting the construction of ExitLimit to drop information if needed. Or maybe that part of your change with an assert in the ExitLimit constructor? Not sure there.
>
> This could work as an intermediate step.  Digging a little, it wouldn't be that painful; there aren't that many places in howFarToZero and howManyLessThans/howManyGreaterThans that would need changes.
>
> But we eventually want something like the current patch, I think.  We don't really want to be doing computations like `getMinusSCEV(LHS, RHS)` where LHS and RHS are pointers. Currently, that returns a SCEV with pointer type, but it doesn't really make sense to treat it as a pointer, particularly in the context of an "icmp eq".

Can I ask you to be incremental here?  I'm not really bothered by your change in computeExitLimit, but I am very bothered by the accessor routine changes.  This is complicated, and I don't think any of us fully understand it.  I'd prefer a series of incremental patches with each one standing on it's own building towards the desired objective.  In particular, I'm worried (mostly on your other patch admittedly) that this intersects with the discussion around pointer providence in subtle ways.  Incrementalism, and thus time to catch mistakes early, seems very worthwhile here.

On the pointer subtraction point, that's a whole different can of worms.  The vectorizers and various loop transforms - I think? - use that idiom for overlap checks.  I agree it's very questionable, but I think that's a deeper issue than "just" SCEV.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103656/new/

https://reviews.llvm.org/D103656