| Message ID | 20201019103624.112920-1-aldyh@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Gracefully handle right shifts larger than the precision. | expand |
On Mon, Oct 19, 2020 at 12:36:24PM +0200, Aldy Hernandez via Gcc-patches wrote: > The test is trying to shift by 129, but the underlying type is 128 bits. > This causes low_bits to wrap around to -1 and things go bad really > quick. > > Attached is my proposed solution. > > Andrew, do you have a preference on how to fix this? Shifts are undefined already when the shift count is equal to the precision. And arbitrarily choosing one of the values as the shift count seems risky to me (and even when used, the most natural would be truncating the excess bits rather than saturating), can't it just punt (return VARYING) for the UB shifts? > > gcc/ChangeLog: > > PR tree-optimization/97488 > * range-op.cc (operator_lshift::op1_range): Handle large right shifts. > > gcc/testsuite/ChangeLog: > > * gcc.dg/pr97488.c: New test. Jakub
On Mon, Oct 19, 2020 at 12:44 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Oct 19, 2020 at 12:36:24PM +0200, Aldy Hernandez via Gcc-patches wrote: > > The test is trying to shift by 129, but the underlying type is 128 bits. > > This causes low_bits to wrap around to -1 and things go bad really > > quick. > > > > Attached is my proposed solution. > > > > Andrew, do you have a preference on how to fix this? > > Shifts are undefined already when the shift count is equal to the precision. > And arbitrarily choosing one of the values as the shift count seems risky to > me (and even when used, the most natural would be truncating the excess bits > rather than saturating), can't it just punt (return VARYING) for the UB > shifts? Good point. It looks cleaner too. How about this? diff --git a/gcc/range-op.cc b/gcc/range-op.cc index 30d2a4d3987..0ab5e62be7f 100644 --- a/gcc/range-op.cc +++ b/gcc/range-op.cc @@ -1579,6 +1579,9 @@ operator_lshift::op1_range (irange &r, wide_int shift = wi::to_wide (shift_amount); if (wi::lt_p (shift, 0, SIGNED)) return false; + if (wi::gt_p (shift, wi::uhwi (TYPE_PRECISION (type), + TYPE_PRECISION (op2.type ())))) + return false; if (shift == 0) { r = lhs;
On Mon, Oct 19, 2020 at 12:55:38PM +0200, Aldy Hernandez wrote: > > Shifts are undefined already when the shift count is equal to the precision. > > And arbitrarily choosing one of the values as the shift count seems risky to > > me (and even when used, the most natural would be truncating the excess bits > > rather than saturating), can't it just punt (return VARYING) for the UB > > shifts? > > Good point. It looks cleaner too. > > How about this? > > diff --git a/gcc/range-op.cc b/gcc/range-op.cc > index 30d2a4d3987..0ab5e62be7f 100644 > --- a/gcc/range-op.cc > +++ b/gcc/range-op.cc > @@ -1579,6 +1579,9 @@ operator_lshift::op1_range (irange &r, > wide_int shift = wi::to_wide (shift_amount); > if (wi::lt_p (shift, 0, SIGNED)) > return false; > + if (wi::gt_p (shift, wi::uhwi (TYPE_PRECISION (type), > + TYPE_PRECISION (op2.type ())))) > + return false; If you use wi::ge_p instead, then LGTM. Jakub
On 10/19/20 1:01 PM, Jakub Jelinek wrote: > On Mon, Oct 19, 2020 at 12:55:38PM +0200, Aldy Hernandez wrote: >>> Shifts are undefined already when the shift count is equal to the precision. >>> And arbitrarily choosing one of the values as the shift count seems risky to >>> me (and even when used, the most natural would be truncating the excess bits >>> rather than saturating), can't it just punt (return VARYING) for the UB >>> shifts? >> >> Good point. It looks cleaner too. >> >> How about this? >> >> diff --git a/gcc/range-op.cc b/gcc/range-op.cc >> index 30d2a4d3987..0ab5e62be7f 100644 >> --- a/gcc/range-op.cc >> +++ b/gcc/range-op.cc >> @@ -1579,6 +1579,9 @@ operator_lshift::op1_range (irange &r, >> wide_int shift = wi::to_wide (shift_amount); >> if (wi::lt_p (shift, 0, SIGNED)) >> return false; >> + if (wi::gt_p (shift, wi::uhwi (TYPE_PRECISION (type), >> + TYPE_PRECISION (op2.type ())))) >> + return false; > > If you use wi::ge_p instead, then LGTM. Thanks. Pushed.
On 10/19/20 6:36 AM, Aldy Hernandez wrote: > The test is trying to shift by 129, but the underlying type is 128 bits. > This causes low_bits to wrap around to -1 and things go bad really > quick. > > Attached is my proposed solution. > > Andrew, do you have a preference on how to fix this? If we know we are shifting by the precision of the object, or more, we cant tell anything about the operand... So just return varying I think. The code is eventually just going to create a mask over all the bits and come up with varying anyway. Andrew
diff --git a/gcc/range-op.cc b/gcc/range-op.cc index 30d2a4d3987..13a0ee37feb 100644 --- a/gcc/range-op.cc +++ b/gcc/range-op.cc @@ -1608,8 +1608,10 @@ operator_lshift::op1_range (irange &r, // This would be [0x42, 0xFC] aka [01000010, 11111100]. // Ideally we do this for each subrange, but just lump them all for now. - unsigned low_bits = TYPE_PRECISION (utype) - - TREE_INT_CST_LOW (shift_amount); + unsigned saturated_shift_amount = TREE_INT_CST_LOW (shift_amount); + if (saturated_shift_amount > TYPE_PRECISION (utype)) + saturated_shift_amount = TYPE_PRECISION (utype); + unsigned low_bits = TYPE_PRECISION (utype) - saturated_shift_amount; wide_int up_mask = wi::mask (low_bits, true, TYPE_PRECISION (utype)); wide_int new_ub = wi::bit_or (up_mask, r.upper_bound ()); wide_int new_lb = wi::set_bit (r.lower_bound (), low_bits); diff --git a/gcc/testsuite/gcc.dg/pr97488.c b/gcc/testsuite/gcc.dg/pr97488.c new file mode 100644 index 00000000000..96dc33cf258 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr97488.c @@ -0,0 +1,11 @@ +// { dg-do compile } +// { dg-options "-O1 -ftree-vrp" } + +__int128 +ef (__int128 ms) +{ + int dh = 129; + int *hj = &dh; + + return ms << *hj ? ms : 0; +}