[PATCH] D152067: [ConstraintElimination] Handle equality predicates

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: nikic.
fhahn added a comment.

Thanks for the patch! Some comments inline



================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:950
 
-  bool Changed = false;
-  if (CSToUse.isConditionImplied(R.Coefficients)) {
+  auto ReplaceCmpWithConstant = [&](CmpInst *Cmp, bool IsTrue) {
     if (!DebugCounter::shouldExecute(EliminatedCounter))
----------------
The changes here are mostly to avoid having to duplicate the code to create True/false constants, and indepdent of the patch, right? Could you split them of into a separate NFC patch?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:955
     LLVM_DEBUG({
-      dbgs() << "Condition " << *Cmp << " implied by dominating constraints\n";
+      dbgs() << "Condition " << (IsTrue ? "" : "!") << *Cmp
+             << " implied by dominating constraints\n";
----------------
It would be nice to print the condition with the negated predicate here (as separate NFC patch)


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:977
+
+  if (R.IsEq) {
+    auto NegatedOrEqual = ConstraintSystem::negateOrEqual(R.Coefficients);
----------------
It would probably be cleaner to move the cod e to check if a constraint is implied to a new member function of `ConstraintTy` and not expose `::isEq` publicly. This should be cleaner overall, especially thinking about adding support for NE as well in the future.


================
Comment at: llvm/test/Transforms/ConstraintElimination/eq.ll:380
+
+define i1 @test_transitivity_of_equality(i64 %a, i64 %b, i64 %c) {
+; CHECK-LABEL: @test_transitivity_of_equality(
----------------
I think we need some additional tests here, including ones where we have something like `a == b && b == c` as facts and `a + 1 == c +1` and various permutations (also checking that `a + 1 == b` is not simplified and so on).

It would also be good to have tests where we  only know things like `a >= b && b >= c` and check that we do not simply *a == c` and so on.


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

https://reviews.llvm.org/D152067