Bounds and alignment analysis through bitwise ops #8574
Conversation
converting it to and from the existing ExprInfo as necessary
| result.mask |= (uint64_t)(-1) << (type.bits() - 1); | ||
| } else if (type.bits() < 64) { | ||
| // Narrow uints are zero-extended. | ||
| result.mask |= (uint64_t)(-1) << type.bits(); |
There was a problem hiding this comment.
Isn't this true regardless of the bounds?
| @@ -496,5 +496,114 @@ bool can_prove(Expr e, const Scope<Interval> &bounds) { | |||
| return is_const_one(e); | |||
| } | |||
|
|
|||
| Simplify::ExprInfo::BitsKnown Simplify::ExprInfo::to_bits_known(const Type &type) const { | |||
There was a problem hiding this comment.
A lot of this bitwise math is a little tricky to follow. Have you thrown this in an SMT solver? I think this should be verified
| // For an int we need to know the sign to know the high bits | ||
| bool sign_bit_known = (known.mask >> (type.bits() - 1)) & 1; | ||
| bool negative = (known.value >> (type.bits() - 1)) & 1; | ||
| if (!sign_bit_known) { |
There was a problem hiding this comment.
These cases need comments explaining each of them
src/Simplify_Call.cpp
Outdated
| if (info && (op->type.is_int() || op->type.is_uint())) { | ||
| auto bits_known = a_info.to_bits_known(op->type) & b_info.to_bits_known(op->type); | ||
| info->from_bits_known(bits_known, op->type); | ||
| if (bits_known.mask == (uint64_t)-1) { |
There was a problem hiding this comment.
Add comment saying something like "This is a constant". Or make ExprInfo::BitsKnown have a helper function like "std::optional<uint64_t> as_constant()`.
There was a problem hiding this comment.
and same below with bitwise_or
| @@ -238,13 +256,24 @@ Expr Simplify::visit(const Call *op, ExprInfo *info) { | |||
| return a | b; | |||
| } | |||
| } else if (op->is_intrinsic(Call::bitwise_not)) { | |||
| Expr a = mutate(op->args[0], nullptr); | |||
| ExprInfo a_info; | |||
| Expr a = mutate(op->args[0], &a_info); | |||
There was a problem hiding this comment.
Why not use the bits_known mask in the same way here that you do for and/or? negated constants are constants
There was a problem hiding this comment.
I was, but then I changed it to not actually compute BitsKnown here.
|
|
||
| Expr unbroadcast = lift_elementwise_broadcasts(op->type, op->name, {a, b}, op->call_type); | ||
| if (unbroadcast.defined()) { | ||
| return mutate(unbroadcast, info); | ||
| } | ||
|
|
||
| if (info && (op->type.is_int() || op->type.is_uint())) { | ||
| auto bits_known = a_info.to_bits_known(op->type) ^ b_info.to_bits_known(op->type); | ||
| info->from_bits_known(bits_known, op->type); |
There was a problem hiding this comment.
same here, check for a constant?
src/Simplify_Internal.h
Outdated
| } | ||
|
|
||
| BitsKnown operator|(const BitsKnown &other) const { | ||
| uint64_t zeros = known_zeros() & other.known_zeros(); |
There was a problem hiding this comment.
Add a comment like in the & case
| rewrite(select(1, x, y), x) || | ||
| rewrite(select(0, x, y), y) || | ||
| rewrite(select(x, y, y), y) || | ||
| (rewrite(select(IRMatcher::likely(true), x, y), true_value) || |
There was a problem hiding this comment.
are these changes meant to be part of this PR? What's going on here?
There was a problem hiding this comment.
Running the fuzzer with debugging turned on identified an issue here where information wasn't being propagated through a constant-folded select as aggressively as it could have been. It happened to have a bitwise op in it, which is why I was looking at the case and puzzling over the too-loose bounds.
Some fuzzing with extra checks turned on revealed that the simplifier would often produce constants without the ExprInfo knowing they were constants. In a few places we were doing a janky remutation of a known constant just to set the info, but this wasn't being consistently done. This commit changes constant-folding in the simplifier and info handling in boolean ops to know that it's a constant value being returned. Also standardize the spelling of zeros
I encountered a case in production where slow code was being generated because Halide didn't recognize
(x + 15) & ~15 - y & ~15as a multiple of 16, so it failed to vectorize an atomic because a GuardWithIf if statement got in the way. This was a new issue for Halide 19, because previously the GuardWithIf if statement was incorrectly being dropped by a now-fixed bug in rfactor.This PR adds support for constant bounds analysis and alignment analysis in the simplifier for bitwise ops. It does this by converting ExprInfo to a more LLVM-style BitsKnown structure temporarily.