[PATCH] D119558: [SCEV] Add SCEVCompareExpr node

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

ping, any chance I can get an LGTM?



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:4153
 
+const SCEV *ScalarEvolution::getCompareExpr(ICmpInst::Predicate Pred,
+                                            const SCEV *LHS,
----------------
mkazantsev wrote:
> I wonder, should we disallow pointer types of LHS and RHS here? I don't see what kind of problems it may bring, just being a bit wary about interpretingo of signed comparisons of pointers.
I actually see one of the advantages of having a scev compare expression as being able to model pointer compares in an analogous way to IR without needing to introduce explicit ptrtoints.  Having said that, I'm entirely happy to disable that for now, and discuss that in a separate review.  I'd largely forgotten about that aspect, and didn't mean to commingle the discussion.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13232
+  case scCompareExpr: {
+    const SCEVCompareExpr *UDiv = cast<SCEVCompareExpr>(S);
+    const SCEV *LHS = UDiv->getLHS(), *RHS = UDiv->getRHS();
----------------
mkazantsev wrote:
> `UDiv -> Cmp`
Agreed, please assume the naming is fixed before landing.


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

https://reviews.llvm.org/D119558