Skip to content

Conversation

@abadams
Copy link
Member

@abadams abadams commented Apr 2, 2024

This is the implementation of lower_rounding_shift_right on main:

Expr lower_rounding_shift_right(const Expr &a, const Expr &b) {
    if (is_positive_const(b)) {
        ...
    }
    // Shift right, then add one to the result if bits were dropped
    // (because b > 0) and the most significant dropped bit was a one.
    Expr b_positive = select(b > 0, make_one(a.type()), make_zero(a.type()));
    return simplify((a >> b) + (b_positive & (a >> (b - 1))));
}

Consider lower_rounding_shift_right(a, (uint8)0)

The term b - 1 becomes 255, and now you have an out-of-range shift,
which causes the simplifier to inject a signed_integer_overflow
intrinsic, and compilation to fail.

This is a little annoying because if b == 0, b_positive is a zero mask,
so the result isn't used anyway (this is also why this change is legal).
In llvm, it's a poison value, not UB, so masking it off works. If the
simplifier were smarter, it might just drop the signed_integer_overflow
intrinsic on detecting that it was being bitwise-and-ed with zero.

But the safest thing to do is not overflow. saturating_add/sub are
typically as cheap as add/sub. 99.9% of the time b is some positive
constant anyway, so it's going to get constant-folded.

abadams added 2 commits April 2, 2024 12:21
Consider `lower_rounding_shift_right(a, (uint8)0)`

The term b - 1 becomes 255, and now you have an out-of-range shift,
which causes the simplifier to inject a signed_integer_overflow
intrinsic, and compilation to fail.

This is a little annoying because if b == 0, b_positive is a zero mask,
so the result isn't used anyway (this is also why this change is legal).
In llvm, it's a poison value, not UB, so masking it off works. If the
simplifier were smarter, it might just drop the signed_integer_overflow
intrinsic on detecting that it was being bitwise-and-ed with zero.

But the safest thing to do is not overflow. saturating_add/sub are
typically as cheap as add/sub. 99.9% of the time b is some positive
constant anyway, so it's going to get constant-folded.
@abadams abadams changed the title Abadams/fix ub in lower rounding shift right fix ub in lower rounding shift right Apr 2, 2024
@abadams abadams merged commit a4158c0 into main Apr 3, 2024
@abadams
Copy link
Member Author

abadams commented Apr 3, 2024

Actually bitmasking a poison value with zero in llvm produces a poison value. This bug just made llvm simplify a (randomly-generated) pipeline to a no-op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants