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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/FindIntrinsics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1274,10 +1274,11 @@ Expr lower_widening_shift_right(const Expr &a, const Expr &b) {
}

Expr lower_rounding_shift_left(const Expr &a, const Expr &b) {
// Shift left, then add one to the result if bits were dropped
// (because b < 0) and the most significant dropped bit was a one.
// Shift left, then add one to the result if bits were dropped (because b < 0)
// and the most significant dropped bit was a one. We must take care not
// to introduce UB in the shifts, even if the result would be masked off.
Expr b_negative = select(b < 0, make_one(a.type()), make_zero(a.type()));
return simplify((a << b) + (b_negative & (a << (b + 1))));
return simplify((a << b) + (b_negative & (a << saturating_add(b, make_one(b.type())))));
}

Expr lower_rounding_shift_right(const Expr &a, const Expr &b) {
Expand All @@ -1289,10 +1290,11 @@ Expr lower_rounding_shift_right(const Expr &a, const Expr &b) {
Expr round = simplify(cast(a.type(), (1 << shift) - 1));
return rounding_halving_add(a, round) >> shift;
}
// Shift right, then add one to the result if bits were dropped
// (because b > 0) and the most significant dropped bit was a one.
// Shift right, then add one to the result if bits were dropped (because b > 0)
// and the most significant dropped bit was a one. We must take care not to
// introduce UB in the shifts, even if the result would be masked off.
Expr b_positive = select(b > 0, make_one(a.type()), make_zero(a.type()));
return simplify((a >> b) + (b_positive & (a >> (b - 1))));
return simplify((a >> b) + (b_positive & (a >> saturating_sub(b, make_one(b.type())))));
}

Expr lower_saturating_add(const Expr &a, const Expr &b) {
Expand Down
16 changes: 16 additions & 0 deletions test/correctness/intrinsics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,22 @@ int main(int argc, char **argv) {
g.compile_jit();
}

// Rounding shifts by extreme values, when lowered, used to have the
// potential to overflow and turn into out-of-range shifts. The simplifier
// detected this and injected a signed_integer_overflow intrinsic, which
// then threw an error in codegen, even though the rounding shift calls are
// well-defined.
{
Func f, g;

f(x) = cast<uint8_t>(x);
f.compute_root();

g(x) = rounding_shift_right(x, 0) + rounding_shift_left(x, 8);

g.compile_jit();
}

printf("Success!\n");
return 0;
}
Loading