[PATCH] D119558: [SCEV] Add SCEVCompareExpr node

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D119558#3315198 <https://reviews.llvm.org/D119558#3315198>, @nikic wrote:

>> I want to highlight that adding the node does not mean we have to extend IR pattern matching. At the moment, I'm planning on using this to simplify and generalize PredicatedScalarEvolution and the loop versioning checks used by various transforms. The decision as to whether we want these in generic SCEV expressions can (and should be) explicitly deferred here.
>
> Could you provide a bit more information on how this is going to be used? It's not completely clear to me whether/why this should be part of the SCEV hierarchy, rather than something that uses SCEV nodes but is not one itself.

So, my current thinking is that SCEVPredicate is just going to end up as a wrapper around a SCEV*.  Currently, we have three predicates types - union, compare, and nowrap.  My immediate plan is to replace the compare with a wrapped SCEV - I should have a patch shortly.  I'd then replace the nowrap variant, and once that's done, do the same for the wrapping union.

I'd originally thought the separate hierarchy make sense as well, but when I started look into it I kept finding myself needing to duplicate logic we already have in SCEV today.  (implication, negation, inference, etc..)  I really do think it makes sense for predicates to just be SCEV expressions.

> Having it as a SCEV makes sense if you want to make it a part of a larger expression, but it's not immediately obvious how that will look in practice (I guess you could use umin/umax as an awkward way to and/or conditions).

We have a way to represent logic or/and in SCEV.  Maybe umin/umax is a bit ugly, but we already have precedent for that.  Having a single set of simplification and inference rules seems strongly better than having two less well tested implementations which largely duplicate functionality.


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

https://reviews.llvm.org/D119558