On Wed, 19 Nov 2014, Jakub Jelinek wrote:
> On Wed, Nov 19, 2014 at 09:59:45AM +0100, Richard Biener wrote:
> > > OImode/XImode on i?86/x86_64 are not <= MAX_BITSIZE_MODE_ANY_INT, because
> > > they are never used for integer arithmetics (and there is no way
> > > to represent all their values in RTL if not using CONST_WIDE_INT).
> > > As the following testcase shows, simplify_immed_subreg can be called
> > > with such modes though, e.g. trying to forward propagate a CONST_VECTOR
> > > (i?86/x86_64 handles all zeros and all ones as CONST_VECTORs that can
> > > appear
> > > in the IL directly) into a SUBREG_REG.
> >
> > So we have (subreg:OI (reg:V4DF ...) ...) for example? What do we
> > end doing with that OI mode subreg? (why can't we simply use the
> > V4DF one?)
>
> propagate_rtx_1 is called on
> (subreg:OI (reg:V8DI 89) 0)
> with old_rtx
> (reg:V8DI 89)
> and new_rtx
> (const_vector:V8DI [
> (const_int 0 [0])
> (const_int 0 [0])
> (const_int 0 [0])
> (const_int 0 [0])
> (const_int 0 [0])
> (const_int 0 [0])
> (const_int 0 [0])
> (const_int 0 [0])
> ])
>
> Seems we throw away the result in that case though, because
> gen_lowpart_common doesn't like to return low parts of VOIDmode constants
> larger if the mode is larger than double int:
> 1353 innermode = GET_MODE (x);
> 1354 if (CONST_INT_P (x)
> 1355 && msize * BITS_PER_UNIT <= HOST_BITS_PER_WIDE_INT)
> 1356 innermode = mode_for_size (HOST_BITS_PER_WIDE_INT, MODE_INT, 0);
> 1357 else if (innermode == VOIDmode)
> 1358 innermode = mode_for_size (HOST_BITS_PER_DOUBLE_INT, MODE_INT, 0);
> we hit the last if and as mode is wider than innermode, we return NULL later
> on.
>
> > > The following patch instead of ICE handles the most common cases (all 0
> > > and all 1 CONST_VECTORs) and returns NULL otherwise.
> > >
> > > Before wide-int got merged, the testcase worked, though the code didn't
> > > bother checking anything, just created 0 or constm1_rtx for the two cases
> > > that could happen and if something else appeared, could just return what
> > > matched low TImode (or DImode for -m32).
> > >
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > Looks ok to me. Not sure if the zero/all-ones case really happens that
> > much to be worth special-casing - I think you could use
> > fixed_wide_int<proper-size> and simply see if the result is representable
> > in a CONST_INT/CONST_DOUBLE? Can you try if that works? It looks like
> > the patch may be smaller for that.
>
> So perhaps something like this? Don't know how much more inefficient it is
> compared to what it used to do before.
Yes, that looks good.
> Or just keep the existing code and just remove the assert and instead return
> NULL whenever outer_submode is wider than MAX_BITSIZE_MODE_ANY_INT? At
> least during propagation that will make zero change.
> Though, in that case I have still doubts about the current code handling right
> modes wider than HOST_BITS_PER_DOUBLE_INT but smaller than
> MAX_BITSIZE_MODE_ANY_INT (none on i?86/x86_64). If TARGET_SUPPORTS_WIDE_INT
> == 0, we still silently throw away the upper bits, don't we?
Well - not with your added check, no?
I'd say the patch is ok.
Thanks,
Richard.
> BTW, to Mike, the assert has been misplaced, there was first buffer overflow
> and only after that the assert.
>
> 2014-11-19 Jakub Jelinek <ja...@redhat.com>
>
> PR target/63910
> * simplify-rtx.c (simplify_immed_subreg): Handle integer modes wider
> than MAX_BITSIZE_MODE_ANY_INT.
>
> * gcc.target/i386/pr63910.c: New test.
>
> --- gcc/simplify-rtx.c.jj 2014-11-19 09:17:15.491327992 +0100
> +++ gcc/simplify-rtx.c 2014-11-19 12:28:16.223808178 +0100
> @@ -5501,8 +5501,12 @@ simplify_immed_subreg (machine_mode oute
> int units
> = (GET_MODE_BITSIZE (outer_submode) + HOST_BITS_PER_WIDE_INT - 1)
> / HOST_BITS_PER_WIDE_INT;
> - HOST_WIDE_INT tmp[MAX_BITSIZE_MODE_ANY_INT /
> HOST_BITS_PER_WIDE_INT];
> - wide_int r;
> + const int tmpsize = (MAX_BITSIZE_MODE_ANY_MODE
> + + HOST_BITS_PER_WIDE_INT - 1)
> + / HOST_BITS_PER_WIDE_INT;
> + HOST_WIDE_INT tmp[tmpsize];
> + typedef FIXED_WIDE_INT (tmpsize * HOST_BITS_PER_WIDE_INT) imm_int;
> + imm_int r;
>
> for (u = 0; u < units; u++)
> {
> @@ -5515,10 +5519,12 @@ simplify_immed_subreg (machine_mode oute
> tmp[u] = buf;
> base += HOST_BITS_PER_WIDE_INT;
> }
> - gcc_assert (GET_MODE_PRECISION (outer_submode)
> - <= MAX_BITSIZE_MODE_ANY_INT);
> - r = wide_int::from_array (tmp, units,
> - GET_MODE_PRECISION (outer_submode));
> + r = imm_int::from_array (tmp, units,
> + GET_MODE_PRECISION (outer_submode));
> +#if TARGET_SUPPORTS_WIDE_INT == 0
> + if (wi::min_precision (r, SIGNED) > HOST_BITS_PER_DOUBLE_INT)
> + return NULL_RTX;
> +#endif
> elems[elem] = immed_wide_int_const (r, outer_submode);
> }
> break;
> --- gcc/testsuite/gcc.target/i386/pr63910.c.jj 2014-11-19
> 12:04:23.490489130 +0100
> +++ gcc/testsuite/gcc.target/i386/pr63910.c 2014-11-19 12:04:23.490489130
> +0100
> @@ -0,0 +1,12 @@
> +/* PR target/63910 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -mstringop-strategy=vector_loop -mavx512f" } */
> +
> +extern void bar (float *c);
> +
> +void
> +foo (void)
> +{
> + float c[1024] = { };
> + bar (c);
> +}
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendoerffer, HRB 21284
(AG Nuernberg)
Maxfeldstrasse 5, 90409 Nuernberg, Germany
