Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ub in lower rounding shift right #8173

Merged
merged 2 commits into from
Apr 3, 2024

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
19 checks passed
@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.

2 participants