[PATCH] D152067: [ConstraintElimination] Handle equality predicates

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ConstraintSystem.h:126
     for (auto &C : R)
       if (MulOverflow(C, int64_t(-1), C))
         return {};
----------------
as follow-up, it would be good to use `negateOrEqual` here as well.


================
Comment at: llvm/include/llvm/Analysis/ConstraintSystem.h:131
 
+  static SmallVector<int64_t, 8> negateOrEqual(SmallVector<int64_t, 8> R) {
+    // The negated constraint R is obtained by multiplying by -1.
----------------
Could you add a doc-comment for the function?


================
Comment at: llvm/include/llvm/Analysis/ConstraintSystem.h:139
+
+  static SmallVector<int64_t, 8> toStrictLessThan(SmallVector<int64_t, 8> R) {
+    // The strict less than is obtained by subtracting 1 from the constant.
----------------
Could you add a doc-comment for the function?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:584
+
+  auto NegatedOrEqual = ConstraintSystem::negateOrEqual(Coefficients);
+  bool IsNegatedOrEqualImplied =
----------------
I think it would probably be better to wrap the `EQ` handling in `IsEq` and then run the general code afterwards.


================
Comment at: llvm/test/Transforms/ConstraintElimination/assumes.ll:636
+  %2 = icmp ult i64 %1, %a
+  tail call void @llvm.assume(i1 %2)
+  %3 = icmp eq i64 %a, %b
----------------
I think those tests would fit better in `eq.ll` as the assume here is only used to add known facts.


================
Comment at: llvm/test/Transforms/ConstraintElimination/eq.ll:411
+  %ac_plus_1_eq = icmp eq i64 %a_plus_1, %c_plus_1
+  %result = select i1 %ac_eq, i1 %ac_plus_1_eq, i1 false
+  ret i1 %result
----------------
simpler to either use `xor` or `and` instead of the more complicated `select`.


================
Comment at: llvm/test/Transforms/ConstraintElimination/reproducer-remarks-debug.ll:12
 ; CHECK-NEXT:   Materializing assumption   %c.1 = icmp eq ptr %a, null
-; CHECK-NEXT: ---
 
----------------
What changed here?


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

https://reviews.llvm.org/D152067