[PATCH] D13687: [SCEV] Opportunistically interpret unsigned constraints as signed

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/comments addressed.

Q: Does InstCombine have similar logic?  If it doesn't, it really should.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:7455
@@ -7454,1 +7454,3 @@
 
+static CmpInst::Predicate GetSignedPredicate(CmpInst::Predicate P) {
+  switch (P) {
----------------
I'm pretty sure this function already exists somewhere.  If it doesn't, it should be a utility on ICmpInst or something.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:7458
@@ +7457,3 @@
+  default:
+    return CmpInst::BAD_ICMP_PREDICATE;
+
----------------
Make this an assert?  I'd rather see the calling code as isUnsignedCmp, followed by a call to this with an assert inside it.  Less error prone.  

================
Comment at: lib/Analysis/ScalarEvolution.cpp:7534
@@ -7514,1 +7533,3 @@
 
+  // Unsigned comparison is the same as signed comparison when both the operands
+  // are non-negative.
----------------
This code is obviously correct, but it only handles one of the two cases immediately above it.  I'd suggest in a separate change, pulling out the two cases above into a lambda, and then calling it twice, once with FoundPred, once with the signed compare if legal.  


http://reviews.llvm.org/D13687