Hi!
On Thu, Feb 18, 2021 at 10:46:11AM -0600, will schmidt wrote:
> On Wed, 2021-02-03 at 03:01 -0600, Xionghu Luo via Gcc-patches wrote:
> > v[k] will also be expanded to IFN VEC_SET if k is long type when
> > built
> > with -Og. -O0 didn't exposed the issue due to v is TREE_ADDRESSABLE,
> > -O1 and above also didn't capture it because of v[k] is not optimized
> > to
> > VIEW_CONVERT_EXPR<int[4]>(v)[k_1].
> > vec_insert defines the element argument type to be signed int by
> > ELFv2
> > ABI, so convert it to SImode if it wasn't for Power target
> > requirements.
>
> The intro paragraph seems to start mid sentence. Did something get cut
> off?
> The description here is specific to the reported testcase failure.
> This should describe the patch behavior instead. Something like
> "When
> expanding a vector with a variable rtx, the rtx type needs to be SI"
> ...
mode, not type :-)
> > gcc/ChangeLog:
>
>
> Reference "PR target/98914" somewhere in here.
Exactly like that, and in *both* changelogs (gcc/ and gcc/testsuite/),
before all entries.
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -7000,8 +7000,6 @@ rs6000_expand_vector_set_var_p9 (rtx target,
> > rtx val, rtx idx)
> >
> > gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> >
> > - gcc_assert (GET_MODE (idx) == E_SImode);
> > -
> > machine_mode inner_mode = GET_MODE (val);
> >
> > rtx tmp = gen_reg_rtx (GET_MODE (idx));
>
>
> This needs a changelog blurb.
Yup... Just "Remove assert." will do.
> > @@ -7144,7 +7140,7 @@ rs6000_expand_vector_set (rtx target, rtx val,
> > rtx elt_rtx)
> > machine_mode mode = GET_MODE (target);
> > machine_mode inner_mode = GET_MODE_INNER (mode);
> > rtx reg = gen_reg_rtx (mode);
> > - rtx mask, mem, x;
> > + rtx mask, mem, x, elt_si;
> > int width = GET_MODE_SIZE (inner_mode);
> > int i;
> >
> > @@ -7154,16 +7150,23 @@ rs6000_expand_vector_set (rtx target, rtx
> > val, rtx elt_rtx)
> > {
> > if (!CONST_INT_P (elt_rtx))
> > {
> > + /* elt_rtx should be SImode from ELFv2 ABI. */
> > + elt_si = gen_reg_rtx (E_SImode);
Declare it only here, not earlier please.
> > + if (GET_MODE (elt_rtx) != E_SImode)
> > + convert_move (elt_si, elt_rtx, 0);
> > + else
> > + elt_si = elt_rtx;
Just always do the convert_move? So:
rtx elt_si = gen_reg_rtx (SImode); // no need for that E_ stuff
convert_move (elt_si, elt_rtx, 0);
This code runs at expand time, so we have many later passes that all can
get rid of that extra move no problem.
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> > @@ -0,0 +1,11 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target powerpc_p8vector_ok } */
> > +/* { dg-options "-Og -mvsx" } */
Please use
-Og -mdejagnu-cpu=power8
instead. Was this tested on BE configs? Please make sure you do.
Please fix (also all Will's other comments, thanks as always!) and
repost (after testing again, of course). Thanks!
Segher
