[PATCH] D13687: [SCEV] Opportunistically interpret unsigned constraints as signed
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 14 11:20:59 PDT 2015
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
More information about the llvm-commits
mailing list