Make the simplifier idempotent (#8840)

* Add a fuzz test for simplifier idempotency

* Remove double simplify in TrimNoOps.cpp

* Add remutation around broadcast simplification

* Improve visual alignment for idempotency error

* Simplify fuzz_simplify.cpp a bit

* Fix fuzz_var_count

* Simplifier changes to try to achieve idempotence

Everywhere we something without remutating, we need to ensure we also
return the ExprInfo that would have resulted from remutation

Also, redid Simplify_EQ to not rely on Simplify_Sub. New rules are
formally verified for infinite integers except for the ones with
broadcast and ramps

* Missed a non-remutating return in Cast

* Don't treat remaining casts as const

* Add missing remutate in abs

* Replace janky retry loop

* Properly initialize RNG (and widen to 64 bit)

* abs sometimes folds selects and ramps to constants

* Fix ramp(ramp()) folding when there is intentional overflow

* Propagate info through promise_clamped

* Add missing EQ rule

* Handle selects on bools in Solve.cpp

Also turn off the infinite loop in the test

* Attempt to dodge inscrutable compiler errors with seed_seq on some platforms

* Overload resolution is hard.

---------

Co-authored-by: Andrew Adams <andrew.b.adams@gmail.com>

Author: alexreinking

src/ConstantInterval.cpp

@@ -456,6 +456,10 @@ ConstantInterval operator%(const ConstantInterval &a, const ConstantInterval &b)
     result.min_defined = true;
     result.min = 0;
 
+    if (a.is_single_point() && b.is_single_point()) {
+        return ConstantInterval::single_point(mod_imp(a.min, b.min));
+    }
+
     // Mod by produces a result between 0
     // and max(0, abs(modulus) - 1). However, if b is unbounded in
     // either direction, abs(modulus) could be arbitrarily

src/IRMatch.h

@@ -391,7 +391,7 @@ std::ostream &operator<<(std::ostream &s, const WildConstFloat<i> &c) {
     return s;
 }
 
-// Matches and binds to any constant Expr. Does not support constant-folding.
+// Matches and binds to any constant Expr.
 template<int i>
 struct WildConst {
     struct pattern_tag {};
@@ -777,6 +777,7 @@ struct CmpOp {
             b.make_folded_const(val_b, ty, state);
             ty.lanes |= l;
         }
+
         switch (ty.code) {
         case halide_type_int:
             val.u.u64 = constant_fold_cmp_op<Op>(val_a.u.i64, val_b.u.i64);

src/IRPrinter.cpp

@@ -1027,18 +1027,46 @@ void IRPrinter::visit(const Broadcast *op) {
 }
 
 void IRPrinter::visit(const Call *op) {
-    // TODO: Print indication of C vs C++?
-    if (!known_type.contains(op->name) &&
-        (op->type != Int(32))) {
-        if (op->type.is_handle()) {
-            stream << type(op->type);  // Already has parens
-        } else {
-            stream << typep(op->type);
+
+    if (op->is_intrinsic(Call::bitwise_or)) {
+        open();
+        print(op->args[0]);
+        stream << paren(" | ");
+        print(op->args[1]);
+        close();
+    } else if (op->is_intrinsic(Call::bitwise_and)) {
+        open();
+        print(op->args[0]);
+        stream << paren(" & ");
+        print(op->args[1]);
+        close();
+    } else if (op->is_intrinsic(Call::bitwise_xor)) {
+        open();
+        print(op->args[0]);
+        stream << paren(" ^ ");
+        print(op->args[1]);
+        close();
+    } else if (op->is_intrinsic(Call::bitwise_not)) {
+        stream << ansi_kw << "~(" << ansi_reset;
+        paren_depth++;
+        print_no_parens(op->args[0]);
+        paren_depth--;
+        stream << kw(")");
+    } else {
+
+        // TODO: Print indication of C vs C++?
+        if (!known_type.contains(op->name) &&
+            (op->type != Int(32))) {
+            if (op->type.is_handle()) {
+                stream << type(op->type);  // Already has parens
+            } else {
+                stream << typep(op->type);
+            }
         }
+        openf(op->name.c_str());
+        print_list(op->args);
+        closef();
     }
-    openf(op->name.c_str());
-    print_list(op->args);
-    closef();
 }
 
 void IRPrinter::visit(const Let *op) {

src/ModulusRemainder.cpp

@@ -365,15 +365,27 @@ void modulus_remainder_test() {
 }
 
 int64_t gcd(int64_t a, int64_t b) {
-    if (a < b) {
-        std::swap(a, b);
+    // We don't care about factors of -1, so we're going to do this unsigned so
+    // that we can take an absolute value without worrying about INT64_MIN.
+    uint64_t ua = (uint64_t)a;
+    uint64_t ub = (uint64_t)b;
+
+    if (a < 0) {
+        ua = -ua;
+    }
+    if (b < 0) {
+        ub = -ub;
+    }
+
+    if (ua < ub) {
+        std::swap(ua, ub);
     }
-    while (b != 0) {
-        int64_t tmp = b;
-        b = a % b;
-        a = tmp;
+    while (ub != 0) {
+        uint64_t tmp = ub;
+        ub = ua % ub;
+        ua = tmp;
     }
-    return a;
+    return (int64_t)(ua);
 }
 
 int64_t lcm(int64_t a, int64_t b) {
@@ -410,13 +422,17 @@ ModulusRemainder operator*(const ModulusRemainder &a, const ModulusRemainder &b)
     if (a.modulus == 0) {
         // a is constant
         if (mul_with_overflow(64, a.remainder, b.modulus, &m) &&
-            mul_with_overflow(64, a.remainder, b.remainder, &r)) {
+            mul_with_overflow(64, a.remainder, b.remainder, &r) &&
+            // a.remainder may have been negative
+            (m > 0 || mul_with_overflow(64, m, -1, &m))) {
             return {m, r};
         }
     } else if (b.modulus == 0) {
         // b is constant
         if (mul_with_overflow(64, a.modulus, b.remainder, &m) &&
-            mul_with_overflow(64, a.remainder, b.remainder, &r)) {
+            mul_with_overflow(64, a.remainder, b.remainder, &r) &&
+            // b.remainder may have been negative
+            (m > 0 || mul_with_overflow(64, m, -1, &m))) {
             return {m, r};
         }
     } else if (a.remainder == 0 && b.remainder == 0) {
@@ -447,6 +463,7 @@ ModulusRemainder operator*(const ModulusRemainder &a, const ModulusRemainder &b)
 }
 
 ModulusRemainder operator/(const ModulusRemainder &a, const ModulusRemainder &b) {
+
     // What can we say about:
     // floor((m1 * x + r1) / (m2 * y + r2))
 
@@ -456,8 +473,13 @@ ModulusRemainder operator/(const ModulusRemainder &a, const ModulusRemainder &b)
     // (m1 / r2) * x + floor(r1 / r2)
     // E.g. (8x + 3) / 2 -> (4x + 1)
 
-    if (b.modulus == 0 && b.remainder != 0) {
-        if (mod(a.modulus, b.remainder) == 0) {
+    if (b.modulus == 0) {
+        if (b.remainder == 0 || b.remainder == INT64_MIN) {
+            return {0, 0};
+        } else if (b.remainder < 0) {
+            // In Euclidean division, a / -b == -(a / b)
+            return ModulusRemainder{0, 0} - (a / -b.remainder);
+        } else if (mod(a.modulus, b.remainder) == 0) {
             int64_t m = a.modulus / b.remainder;
             int64_t r = mod(div_imp(a.remainder, b.remainder), m);
             return {m, r};
@@ -552,6 +574,7 @@ ModulusRemainder operator%(const ModulusRemainder &a, const ModulusRemainder &b)
     // 2w + 1
     int64_t modulus = gcd(a.modulus, b.modulus);
     modulus = gcd(modulus, b.remainder);
+
     int64_t remainder = mod(a.remainder, modulus);
 
     if (b.remainder == 0 && remainder != 0) {

src/ModulusRemainder.h

@@ -91,7 +91,8 @@ HALIDE_MUST_USE_RESULT bool reduce_expr_modulo(const Expr &e, int64_t modulus, i
 
 void modulus_remainder_test();
 
-/** The greatest common divisor of two integers */
+/** The greatest common divisor of two integers. Returns a positive result,
+ * unless both args are INT64_MIN. */
 int64_t gcd(int64_t, int64_t);
 
 /** The least common multiple of two integers */

src/Simplify.cpp

@@ -526,53 +526,49 @@ Simplify::ExprInfo::BitsKnown Simplify::ExprInfo::to_bits_known(const Type &type
             // the alignment analysis catches constants at the same time as
             // bounds analysis does.
             return (uint64_t)-1;
+        } else if ((int64_t)x < 0) {
+            // There are no leading zeros, but we can't shift left by 64
+            return (uint64_t)0;
         }
         return (uint64_t)(-1) << (64 - clz64(x));
     };
 
-    auto leading_ones_mask = [=](uint64_t x) {
-        return leading_zeros_mask(~x);
-    };
-
-    // The bounds and the type tell us a bunch of high bits are zero or one
-    if (type.is_uint()) {
-        // Narrow uints are always zero-extended.
-        if (type.bits() < 64) {
-            result.mask |= (uint64_t)(-1) << type.bits();
-        }
-        // A lower bound might tell us that there are some leading ones, and an
-        // upper bound might tell us that there are some leading
-        // zeros. Unfortunately we'll never learn about leading ones, because to
-        // know that there's a leading one from the bounds would require knowing
-        // that the min is at least 2^63, and ConstantInterval can't represent
-        // mins that large.
-        if (bounds.max_defined) {
-            result.mask |= leading_zeros_mask(bounds.max);
-        }
+    if (bounds.min_defined && bounds.max_defined) {
+        // Any leading bits in common between the min and the max are known.
+        result.mask |= leading_zeros_mask(bounds.min ^ bounds.max);
+        result.value |= bounds.min & result.mask;
     } else {
-        internal_assert(type.is_int());
-        // A mask which is 1 for the sign bit and above.
-        uint64_t sign_bit_and_above = (uint64_t)(-1) << (type.bits() - 1);
-        if (bounds >= 0) {
-            // We know this int is positive, so the sign bit and above are zero.
-            result.mask |= sign_bit_and_above;
+        // If we only have a bound on one side, we may still be able to infer
+        // something about high bits.
+
+        // The bounds and the type tell us a bunch of high bits are zero or one
+        if (type.is_uint()) {
+            // Narrow uints are always zero-extended.
+            if (type.bits() < 64) {
+                result.mask |= (uint64_t)(-1) << type.bits();
+            }
+
+            // A lower bound might tell us that there are some leading ones, and an
+            // upper bound might tell us that there are some leading
+            // zeros. Unfortunately we'll never learn about leading ones, because to
+            // know that there's a leading one from the bounds would require knowing
+            // that the min is at least 2^63, and ConstantInterval can't represent
+            // mins that large.
             if (bounds.max_defined) {
-                // We also have an upper bound, so there may be more zero bits,
-                // depending on how many leading zeros there are in the upper
-                // bound.
                 result.mask |= leading_zeros_mask(bounds.max);
             }
-        } else if (bounds < 0) {
-            // This int is negative, so the sign bit and above are one.
-            result.mask |= sign_bit_and_above;
-            result.value |= sign_bit_and_above;
-            if (bounds.min_defined) {
-                // We have a lower bound, so there may be more leading one bits,
-                // depending on how many leading ones there are in the lower
-                // bound.
-                uint64_t leading_ones = leading_ones_mask(bounds.min);
-                result.mask |= leading_ones;
-                result.value |= leading_ones;
+
+        } else {
+            internal_assert(type.is_int());
+            // A mask which is 1 for the sign bit and above.
+            uint64_t sign_bit_and_above = (uint64_t)(-1) << (type.bits() - 1);
+            if (bounds >= 0) {
+                // We know this int is positive, so the sign bit and above are zero.
+                result.mask |= sign_bit_and_above;
+            } else if (bounds < 0) {
+                // This int is negative, so the sign bit and above are one.
+                result.mask |= sign_bit_and_above;
+                result.value |= sign_bit_and_above;
             }
         }
     }

src/Simplify_Add.cpp

@@ -11,8 +11,8 @@ Expr Simplify::visit(const Add *op, ExprInfo *info) {
     if (info) {
         info->bounds = a_info.bounds + b_info.bounds;
         info->alignment = a_info.alignment + b_info.alignment;
-        info->trim_bounds_using_alignment();
         info->cast_to(op->type);
+        info->trim_bounds_using_alignment();
     }
 
     // Order commutative operations by node type

src/Simplify_And.cpp

@@ -11,6 +11,10 @@ Expr Simplify::visit(const And *op, ExprInfo *info) {
     Expr a = mutate(op->a, nullptr);
     Expr b = mutate(op->b, nullptr);
 
+    if (info && is_const_one(a) && is_const_one(b)) {
+        return const_true(op->type.lanes(), info);
+    }
+
     // Order commutative operations by node type
     if (should_commute(a, b)) {
         std::swap(a, b);
@@ -27,7 +31,7 @@ Expr Simplify::visit(const And *op, ExprInfo *info) {
     // Cases that fold to a constant
     if (EVAL_IN_LAMBDA
         (rewrite(x && false, false) ||
-
+         rewrite(false && x, false) ||
          rewrite(x && neg(x), false) ||
          rewrite(x && (neg(x) && y), false) ||
          rewrite(x && (y && neg(x)), false) ||
@@ -80,7 +84,6 @@ Expr Simplify::visit(const And *op, ExprInfo *info) {
     // Cases that fold to one of the args
     if (EVAL_IN_LAMBDA
         (rewrite(x && true, a) ||
-
          rewrite(x && x, a) ||
          rewrite(x && (x && y), b) ||
          rewrite(x && (y && x), b) ||