On Tue, 9 May 2023, Jakub Jelinek wrote: > Hi! > > The following testcase is miscompiled, because bitwise ccp2 handles > a rotate with a signed type incorrectly. > Seems tree-ssa-ccp.cc has the only callers of wi::[lr]rotate with 3 > arguments, all other callers just rotate in the right precision and > I think work correctly. ccp works with widest_ints and so rotations > by the excessive precision certainly don't match what it wants > when it sees a rotate in some specific bitsize. Still, if it is > unsigned rotate and the widest_int is zero extended from width, > the functions perform left shift and logical right shift on the value > and then at the end zero extend the result of left shift and uselessly > also the result of logical right shift and return | of that. > On the testcase we the signed char rrotate by 4 argument is > CONSTANT -75 i.e. 0xffffffff....fffffb5 with mask 2. > The mask is correctly rotated to 0x20, but because the 8-bit constant > is sign extended to 192-bit one, the logical right shift by 4 doesn't > yield expected 0xb, but gives 0xfffffffffff....ffffb, and then > return wi::zext (left, width) | wi::zext (right, width); where left is > 0xfffffff....fb50, so we return 0xfb instead of the expected > 0x5b. > > The following patch fixes that by doing the zero extension in case of > the right variable before doing wi::lrshift rather than after it. > > Also, wi::[lr]rotate widht width < precision always zero extends > the result. I'm afraid it can't do better because it doesn't know > if it is done for an unsigned or signed type, but the caller in this > case knows that very well, so I've done the extension based on sgn > in the caller. E.g. 0x5b rotated right (or left) by 4 with width 8 > previously gave 0xb5, but sgn == SIGNED in widest_int it should be > 0xffffffff....fffb5 instead. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk > and release branches?
OK. Thanks, Richard. > 2023-05-09 Jakub Jelinek <ja...@redhat.com> > > PR tree-optimization/109778 > * wide-int.h (wi::lrotate, wi::rrotate): Call wi::lrshift on > wi::zext (x, width) rather than x if width != precision, rather > than using wi::zext (right, width) after the shift. > * tree-ssa-ccp.cc (bit_value_binop): Call wi::ext on the results > of wi::lrotate or wi::rrotate. > > * gcc.c-torture/execute/pr109778.c: New test. > > --- gcc/wide-int.h.jj 2023-04-18 11:00:39.926725744 +0200 > +++ gcc/wide-int.h 2023-05-08 23:36:41.104412818 +0200 > @@ -3187,9 +3187,11 @@ wi::lrotate (const T1 &x, const T2 &y, u > width = precision; > WI_UNARY_RESULT (T2) ymod = umod_trunc (y, width); > WI_UNARY_RESULT (T1) left = wi::lshift (x, ymod); > - WI_UNARY_RESULT (T1) right = wi::lrshift (x, wi::sub (width, ymod)); > + WI_UNARY_RESULT (T1) right > + = wi::lrshift (width != precision ? wi::zext (x, width) : x, > + wi::sub (width, ymod)); > if (width != precision) > - return wi::zext (left, width) | wi::zext (right, width); > + return wi::zext (left, width) | right; > return left | right; > } > > @@ -3204,10 +3206,11 @@ wi::rrotate (const T1 &x, const T2 &y, u > if (width == 0) > width = precision; > WI_UNARY_RESULT (T2) ymod = umod_trunc (y, width); > - WI_UNARY_RESULT (T1) right = wi::lrshift (x, ymod); > + WI_UNARY_RESULT (T1) right > + = wi::lrshift (width != precision ? wi::zext (x, width) : x, ymod); > WI_UNARY_RESULT (T1) left = wi::lshift (x, wi::sub (width, ymod)); > if (width != precision) > - return wi::zext (left, width) | wi::zext (right, width); > + return wi::zext (left, width) | right; > return left | right; > } > > --- gcc/tree-ssa-ccp.cc.jj 2023-01-02 09:32:39.990030918 +0100 > +++ gcc/tree-ssa-ccp.cc 2023-05-09 00:03:02.692915316 +0200 > @@ -1552,6 +1552,8 @@ bit_value_binop (enum tree_code code, si > *mask = wi::lrotate (r1mask, shift, width); > *val = wi::lrotate (r1val, shift, width); > } > + *mask = wi::ext (*mask, width, sgn); > + *val = wi::ext (*val, width, sgn); > } > } > else if (wi::ltu_p (r2val | r2mask, width) > @@ -1593,8 +1595,8 @@ bit_value_binop (enum tree_code code, si > /* Accumulate the result. */ > res_mask |= tmp_mask | (res_val ^ tmp_val); > } > - *val = wi::bit_and_not (res_val, res_mask); > - *mask = res_mask; > + *val = wi::ext (wi::bit_and_not (res_val, res_mask), width, sgn); > + *mask = wi::ext (res_mask, width, sgn); > } > break; > > --- gcc/testsuite/gcc.c-torture/execute/pr109778.c.jj 2023-05-09 > 00:05:20.249959226 +0200 > +++ gcc/testsuite/gcc.c-torture/execute/pr109778.c 2023-05-09 > 00:04:58.870263249 +0200 > @@ -0,0 +1,26 @@ > +/* PR tree-optimization/109778 */ > + > +int a, b, c, d, *e = &c; > + > +static inline unsigned > +foo (unsigned char x) > +{ > + x = 1 | x << 1; > + x = x >> 4 | x << 4; > + return x; > +} > + > +static inline void > +bar (unsigned x) > +{ > + *e = 8 > foo (x + 86) - 86; > +} > + > +int > +main () > +{ > + d = a && b; > + bar (d + 4); > + if (c != 1) > + __builtin_abort (); > +} > > Jakub > > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)