| Message ID | 20160212132813.GR3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 12 Feb 2016, Jakub Jelinek wrote: > On Fri, Feb 12, 2016 at 01:50:08PM +0100, Richard Biener wrote: > > > - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; > > > - if (xmode1 != VOIDmode && xmode1 != mode1) > > > + mode1 = GET_MODE (xop1); > > > + if (xmode1 != mode1) > > > { > > > xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); > > > mode1 = xmode1; > > > > > > so if xop1 is not VOIDmode and already of xmode1 we won't do anything. > > > But if it is VOIDmode (and xmode1 is not VOIDmode) we'll always > > > do convert_modes. > > > > > > The only thing that can (hopefully not) happen is that xmode1 > > > is VOIDmode but mode1 is not (maybe removing the xmode1 != VOIDmode > > > check is a bit too optimistic here). Not sure if there can be > > > valid patterns requesting a VOIDmode op ... (and not only accept > > > CONST_INTs). > > > > Oh, bootstrap & testing went fine on x86_64-unknown-linux-gnu. > > > > If we can agree on the patch then I'll pick up the testcase from > > your patch (and adjust mine). > > Another possibility, only do the convert_modes from VOIDmode for > shift_optab_p's xop1, and keep doing what we've done before otherwise. That looks like a very targeted and safe fix indeed. Richard. > 2016-02-12 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/69764 > PR rtl-optimization/69771 > * optabs.c (expand_binop_directly): For shift_optab_p, force > convert_modes with VOIDmode if xop1 has VOIDmode. > > * c-c++-common/pr69764.c: New test. > * gcc.dg/torture/pr69771.c: New testcase. > > --- gcc/optabs.c.jj 2016-02-12 00:50:30.410240366 +0100 > +++ gcc/optabs.c 2016-02-12 10:33:32.743492532 +0100 > @@ -988,7 +988,7 @@ expand_binop_directly (machine_mode mode > from_mode, 1); > machine_mode xmode0 = insn_data[(int) icode].operand[1].mode; > machine_mode xmode1 = insn_data[(int) icode].operand[2].mode; > - machine_mode mode0, mode1, tmp_mode; > + machine_mode mode0, mode1 = mode, tmp_mode; > struct expand_operand ops[3]; > bool commutative_p; > rtx_insn *pat; > @@ -1006,6 +1006,8 @@ expand_binop_directly (machine_mode mode > xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp); > if (!shift_optab_p (binoptab)) > xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp); > + else > + mode1 = VOIDmode; > > /* In case the insn wants input operands in modes different from > those of the actual operands, convert the operands. It would > @@ -1020,7 +1020,7 @@ expand_binop_directly (machine_mode mode > mode0 = xmode0; > } > > - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; > + mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1; > if (xmode1 != VOIDmode && xmode1 != mode1) > { > xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); > --- gcc/testsuite/c-c++-common/pr69764.c.jj 2016-02-12 10:27:34.522587995 +0100 > +++ gcc/testsuite/c-c++-common/pr69764.c 2016-02-12 10:27:34.522587995 +0100 > @@ -0,0 +1,38 @@ > +/* PR rtl-optimization/69764 */ > +/* { dg-do compile { target int32plus } } */ > + > +unsigned char > +fn1 (unsigned char a) > +{ > + return a >> ~6; /* { dg-warning "right shift count is negative" } */ > +} > + > +unsigned short > +fn2 (unsigned short a) > +{ > + return a >> ~6; /* { dg-warning "right shift count is negative" } */ > +} > + > +unsigned int > +fn3 (unsigned int a) > +{ > + return a >> ~6; /* { dg-warning "right shift count is negative" } */ > +} > + > +unsigned char > +fn4 (unsigned char a) > +{ > + return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */ > +} > + > +unsigned short > +fn5 (unsigned short a) > +{ > + return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */ > +} > + > +unsigned int > +fn6 (unsigned int a) > +{ > + return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */ > +} > --- gcc/testsuite/gcc.dg/torture/pr69771.c.jj 2016-02-12 10:27:34.522587995 +0100 > +++ gcc/testsuite/gcc.dg/torture/pr69771.c 2016-02-12 10:27:34.522587995 +0100 > @@ -0,0 +1,12 @@ > +/* PR rtl-optimization/69771 */ > +/* { dg-do compile } */ > + > +unsigned char a = 5, c; > +unsigned short b = 0; > +unsigned d = 0x76543210; > + > +void > +foo (void) > +{ > + c = d >> ~(a || ~b); /* { dg-warning "shift count is negative" } */ > +} > > Jakub > >
On 02/12/2016 02:47 PM, Richard Biener wrote: >> Another possibility, only do the convert_modes from VOIDmode for >> shift_optab_p's xop1, and keep doing what we've done before otherwise. > > That looks like a very targeted and safe fix indeed. You two can obviously go ahead and sort this out, I'll just make one more comment. What attracted me to Richard's patch was its clarity and lack of potential to confuse readers. >> - machine_mode mode0, mode1, tmp_mode; >> + machine_mode mode0, mode1 = mode, tmp_mode; >> if (!shift_optab_p (binoptab)) >> xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp); >> + else >> + mode1 = VOIDmode; >> - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; >> + mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1; >> if (xmode1 != VOIDmode && xmode1 != mode1) >> { Here, things aren't so clear, and the fact that the mode1 calculation now differs from the mode0 one may be overlooked by someone in the future. Rather than use codes like "mode variable is VOIDmode", I'd prefer to use booleans with descriptive names, like "op1_may_need_conversion". Bernd
--- gcc/optabs.c.jj 2016-02-12 00:50:30.410240366 +0100 +++ gcc/optabs.c 2016-02-12 10:33:32.743492532 +0100 @@ -988,7 +988,7 @@ expand_binop_directly (machine_mode mode from_mode, 1); machine_mode xmode0 = insn_data[(int) icode].operand[1].mode; machine_mode xmode1 = insn_data[(int) icode].operand[2].mode; - machine_mode mode0, mode1, tmp_mode; + machine_mode mode0, mode1 = mode, tmp_mode; struct expand_operand ops[3]; bool commutative_p; rtx_insn *pat; @@ -1006,6 +1006,8 @@ expand_binop_directly (machine_mode mode xop0 = avoid_expensive_constant (xmode0, binoptab, 0, xop0, unsignedp); if (!shift_optab_p (binoptab)) xop1 = avoid_expensive_constant (xmode1, binoptab, 1, xop1, unsignedp); + else + mode1 = VOIDmode; /* In case the insn wants input operands in modes different from those of the actual operands, convert the operands. It would @@ -1020,7 +1020,7 @@ expand_binop_directly (machine_mode mode mode0 = xmode0; } - mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; + mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode1; if (xmode1 != VOIDmode && xmode1 != mode1) { xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); --- gcc/testsuite/c-c++-common/pr69764.c.jj 2016-02-12 10:27:34.522587995 +0100 +++ gcc/testsuite/c-c++-common/pr69764.c 2016-02-12 10:27:34.522587995 +0100 @@ -0,0 +1,38 @@ +/* PR rtl-optimization/69764 */ +/* { dg-do compile { target int32plus } } */ + +unsigned char +fn1 (unsigned char a) +{ + return a >> ~6; /* { dg-warning "right shift count is negative" } */ +} + +unsigned short +fn2 (unsigned short a) +{ + return a >> ~6; /* { dg-warning "right shift count is negative" } */ +} + +unsigned int +fn3 (unsigned int a) +{ + return a >> ~6; /* { dg-warning "right shift count is negative" } */ +} + +unsigned char +fn4 (unsigned char a) +{ + return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */ +} + +unsigned short +fn5 (unsigned short a) +{ + return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */ +} + +unsigned int +fn6 (unsigned int a) +{ + return a >> 0xff03; /* { dg-warning "right shift count >= width of type" } */ +} --- gcc/testsuite/gcc.dg/torture/pr69771.c.jj 2016-02-12 10:27:34.522587995 +0100 +++ gcc/testsuite/gcc.dg/torture/pr69771.c 2016-02-12 10:27:34.522587995 +0100 @@ -0,0 +1,12 @@ +/* PR rtl-optimization/69771 */ +/* { dg-do compile } */ + +unsigned char a = 5, c; +unsigned short b = 0; +unsigned d = 0x76543210; + +void +foo (void) +{ + c = d >> ~(a || ~b); /* { dg-warning "shift count is negative" } */ +}