This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- From: Richard Biener <rguenther at suse dot de>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Bernd Schmidt <bschmidt at redhat dot com>, gcc-patches at gcc dot gnu dot org
- Date: Fri, 12 Feb 2016 14:47:14 +0100 (CET)
- Subject: Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot LSU dot 2 dot 11 dot 1602121119020 dot 31122 at t29 dot fhfr dot qr> <20160212103203 dot GM3017 at tucnak dot redhat dot com> <alpine dot LSU dot 2 dot 11 dot 1602121145370 dot 31122 at t29 dot fhfr dot qr> <56BDCCCF dot 2030601 at redhat dot com> <20160212122438 dot GP3017 at tucnak dot redhat dot com> <alpine dot LSU dot 2 dot 11 dot 1602121346100 dot 31122 at t29 dot fhfr dot qr> <alpine dot LSU dot 2 dot 11 dot 1602121349340 dot 31122 at t29 dot fhfr dot qr> <20160212132813 dot GR3017 at tucnak dot redhat dot com>
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
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)