[rs6000,expand,hooks] : Fix PR98872, handle uninitialized opaque mode variables

Adding Richard since he's reviewed the generic opaque mode code in
the past and this patch contains some more eneric support.

GCC handles pseudos that are used uninitialized, by emitting a
(set (reg: <reg>) CONST0_RTX(regmode)) before their uninitialized
pseudo usage.  Currently, CONST0_RTX(mode) is NULL for opaque modes,
which leads to an ICE.  The solution is to enhance init_emit_once() to
add initialization of CONST0_RTX for opaque modes using a target hook,
since CONST0_RTX probably are different for each opaque mode and each
target.  The default hook throws an error to force the target to think
hard about what their CONST0_RTX values should be for each mode.

This passed bootstrap and regtesting on x86_64-linux and powerpc64le-linux
with no regressions.  Ok for mainline?

Peter


gcc/
	PR target/98872
	* config/rs6000/mma.md (*movoo): Accept zero constraint.
	(mma_xxsetaccz): Use CONST0_RTX.
	* config/rs6000/predicates.md: Recognize OOmode CONST0_RTX.
	* config/rs6000/rs6000.c (TARGET_OPAQUE_CONST0_RTX): Define.
	(rs6000_split_multireg_move): Handle splitting an OOmode register
	set to CONST0_RTX.
	(rs6000_opaque_const0_rtx): New function.
	* emit-rtl.c (init_emit_once): Initialize CONST0_RTX for opaque modes.
	* hooks.c (hook_rtx_mode_unreachable): New function.
	* hooks.h (hook_rtx_mode_unreachable): Declare
	* target.def (opaque_const0_rtx): New target hook.
	* doc/tm.texi.in: Document it.
	* doc/tm.texi: Regenerate.

gcc/testsuite/
	* gcc.target/powerpc/pr98872.c: New test.

Peter Bergner <bergner@linux.ibm.com> writes:
> Adding Richard since he's reviewed the generic opaque mode code in
> the past and this patch contains some more eneric support.
>
> GCC handles pseudos that are used uninitialized, by emitting a
> (set (reg: <reg>) CONST0_RTX(regmode)) before their uninitialized
> pseudo usage.  Currently, CONST0_RTX(mode) is NULL for opaque modes,
> which leads to an ICE.  The solution is to enhance init_emit_once() to
> add initialization of CONST0_RTX for opaque modes using a target hook,
> since CONST0_RTX probably are different for each opaque mode and each
> target.  The default hook throws an error to force the target to think
> hard about what their CONST0_RTX values should be for each mode.

Hmm, but it looks like the hook uses const0_rtx, i.e. a const_int,
for something that isn't an integer mode.  Also, the unspec for XOmode
isn't a constant in the normal sense (CONSTANT_P).

I think we should either add a new rtx code for constant opaque modes
or make init-regs just emit the clobber for opaque modes (and not emit
the move).

Thanks,
Richard

>
> This passed bootstrap and regtesting on x86_64-linux and powerpc64le-linux
> with no regressions.  Ok for mainline?
>
> Peter
>
>
> gcc/
> 	PR target/98872
> 	* config/rs6000/mma.md (*movoo): Accept zero constraint.
> 	(mma_xxsetaccz): Use CONST0_RTX.
> 	* config/rs6000/predicates.md: Recognize OOmode CONST0_RTX.
> 	* config/rs6000/rs6000.c (TARGET_OPAQUE_CONST0_RTX): Define.
> 	(rs6000_split_multireg_move): Handle splitting an OOmode register
> 	set to CONST0_RTX.
> 	(rs6000_opaque_const0_rtx): New function.
> 	* emit-rtl.c (init_emit_once): Initialize CONST0_RTX for opaque modes.
> 	* hooks.c (hook_rtx_mode_unreachable): New function.
> 	* hooks.h (hook_rtx_mode_unreachable): Declare
> 	* target.def (opaque_const0_rtx): New target hook.
> 	* doc/tm.texi.in: Document it.
> 	* doc/tm.texi: Regenerate.
>
> gcc/testsuite/
> 	* gcc.target/powerpc/pr98872.c: New test.
>
> diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md
> index 87569f1c31d..fab849a4f12 100644
> --- a/gcc/config/rs6000/mma.md
> +++ b/gcc/config/rs6000/mma.md
> @@ -272,7 +272,7 @@
>  
>  (define_insn_and_split "*movoo"
>    [(set (match_operand:OO 0 "nonimmediate_operand" "=wa,m,wa")
> -	(match_operand:OO 1 "input_operand" "m,wa,wa"))]
> +	(match_operand:OO 1 "input_operand" "m,wa,waO"))]
>    "TARGET_MMA
>     && (gpc_reg_operand (operands[0], OOmode)
>         || gpc_reg_operand (operands[1], OOmode))"
> @@ -473,9 +473,7 @@
>  	(const_int 0))]
>    "TARGET_MMA"
>  {
> -  rtx xo0 = gen_rtx_UNSPEC (XOmode, gen_rtvec (1, const0_rtx),
> -			    UNSPEC_MMA_XXSETACCZ);
> -  emit_insn (gen_rtx_SET (operands[0], xo0));
> +  emit_insn (gen_rtx_SET (operands[0], CONST0_RTX (XOmode)));
>    DONE;
>  })
>  
> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
> index 5d1952e59d3..30805ab0619 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -1081,6 +1081,10 @@
>        && easy_vector_constant (op, mode))
>      return 1;
>  
> +  /* For OOmode, zero is an easy constant.  */
> +  if (mode == OOmode && op == CONST0_RTX (mode))
> +    return 1;
> +
>    /* For floating-point or multi-word mode, the only remaining valid type
>       is a register.  */
>    if (SCALAR_FLOAT_MODE_P (mode)
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index f5565a1a253..c726aa09d26 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -1752,6 +1752,10 @@ static const struct attribute_spec rs6000_attribute_table[] =
>  
>  #undef TARGET_INVALID_CONVERSION
>  #define TARGET_INVALID_CONVERSION rs6000_invalid_conversion
> +
> +#undef TARGET_OPAQUE_CONST0_RTX
> +#define TARGET_OPAQUE_CONST0_RTX rs6000_opaque_const0_rtx
> +
>  
>  
>  /* Processor table.  */
> @@ -16624,6 +16628,19 @@ rs6000_split_multireg_move (rtx dst, rtx src)
>  	  return;
>  	}
>  
> +      /* Split the clearing of an OOmode register pair into clearing
> +	 of its two constituent registers.  */
> +      if (REG_P (dst) && mode == OOmode && src == CONST0_RTX (mode))
> +	{
> +	  int regno = REGNO (dst);
> +	  gcc_assert (VSX_REGNO_P (regno));
> +	  emit_insn (gen_rtx_SET (gen_rtx_REG (reg_mode, regno),
> +				  CONST0_RTX (reg_mode)));
> +	  emit_insn (gen_rtx_SET (gen_rtx_REG (reg_mode, regno + 1),
> +				  CONST0_RTX (reg_mode)));
> +	  return;
> +	}
> +
>        /* Register -> register moves can use common code.  */
>      }
>  
> @@ -27477,6 +27494,25 @@ rs6000_output_addr_vec_elt (FILE *file, int value)
>    fprintf (file, "\n");
>  }
>  
> +/* Implement TARGET_OPAQUE_CONST0_RTX.  */
> +
> +rtx
> +rs6000_opaque_const0_rtx (machine_mode mode)
> +{
> +  gcc_assert (OPAQUE_MODE_P (mode));
> +
> +  switch (mode)
> +    {
> +    case E_OOmode:
> +      return const0_rtx;
> +    case E_XOmode:
> +      return gen_rtx_UNSPEC (XOmode, gen_rtvec (1, const0_rtx),
> +			     UNSPEC_MMA_XXSETACCZ);
> +    default:
> +      gcc_unreachable ();
> +    }
> +}
> +
>  struct gcc_target targetm = TARGET_INITIALIZER;
>  
>  #include "gt-rs6000.h"
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index b23284e5e56..4ac26491481 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -6408,6 +6408,11 @@ init_emit_once (void)
>      if (GET_MODE_CLASS ((machine_mode) i) == MODE_CC)
>        const_tiny_rtx[0][i] = const0_rtx;
>  
> +  FOR_EACH_MODE_IN_CLASS (mode, MODE_OPAQUE)
> +    {
> +      const_tiny_rtx[0][(int) mode] = targetm.opaque_const0_rtx (mode);
> +    }
> +
>    pc_rtx = gen_rtx_fmt_ (PC, VOIDmode);
>    ret_rtx = gen_rtx_fmt_ (RETURN, VOIDmode);
>    simple_return_rtx = gen_rtx_fmt_ (SIMPLE_RETURN, VOIDmode);
> diff --git a/gcc/hooks.c b/gcc/hooks.c
> index 680271f76a4..8e71105265a 100644
> --- a/gcc/hooks.c
> +++ b/gcc/hooks.c
> @@ -380,6 +380,12 @@ hook_bool_wint_wint_uint_bool_true (const widest_int &, const widest_int &,
>    return true;
>  }
>  
> +rtx
> +hook_rtx_mode_unreachable (machine_mode mode ATTRIBUTE_UNUSED)
> +{
> +  gcc_unreachable ();
> +}
> +
>  /* Generic hook that takes an rtx and returns it.  */
>  rtx
>  hook_rtx_rtx_identity (rtx x)
> diff --git a/gcc/hooks.h b/gcc/hooks.h
> index add9a742e41..ba9b6d9ff75 100644
> --- a/gcc/hooks.h
> +++ b/gcc/hooks.h
> @@ -119,6 +119,7 @@ extern unsigned int hook_uint_mode_0 (machine_mode);
>  extern bool default_can_output_mi_thunk_no_vcall (const_tree, HOST_WIDE_INT,
>  						  HOST_WIDE_INT, const_tree);
>  
> +extern rtx hook_rtx_mode_unreachable (machine_mode);
>  extern rtx hook_rtx_rtx_identity (rtx);
>  extern rtx hook_rtx_rtx_null (rtx);
>  extern rtx hook_rtx_tree_int_null (tree, int);
> diff --git a/gcc/target.def b/gcc/target.def
> index be7fcde961a..c026e584c78 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -3362,6 +3362,13 @@ constants can be done inline.  The function\n\
>   HOST_WIDE_INT, (const_tree constant, HOST_WIDE_INT basic_align),
>   default_constant_alignment)
>  
> +DEFHOOK
> +(opaque_const0_rtx,
> + "Return an RTX representing the value @code{0} for opaque mode @var{mode}.\n\
> +The default version of this hook always throws an error.",
> +rtx, (machine_mode mode),
> +hook_rtx_mode_unreachable)
> +
>  DEFHOOK
>  (translate_mode_attribute,
>   "Define this hook if during mode attribute processing, the port should\n\
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 3b19e6f4281..74f6626112b 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -3361,6 +3361,8 @@ stack.
>  
>  @hook TARGET_REF_MAY_ALIAS_ERRNO
>  
> +@hook TARGET_OPAQUE_CONST0_RTX
> +
>  @hook TARGET_TRANSLATE_MODE_ATTRIBUTE
>  
>  @hook TARGET_SCALAR_MODE_SUPPORTED_P
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr98872.c b/gcc/testsuite/gcc.target/powerpc/pr98872.c
> new file mode 100644
> index 00000000000..580b90de7dc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr98872.c
> @@ -0,0 +1,20 @@
> +/* PR target/98872 */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> +
> +/* Verify we do not ICE on the tests below.  */
> +
> +void
> +foo (__vector_quad *dst)
> +{
> +  __vector_quad acc;
> +  *dst = acc;
> +}
> +
> +void
> +bar (__vector_pair *dst)
> +{
> +  __vector_pair pair;
> +  *dst = pair;
> +}

Hi!

On Mon, Feb 08, 2021 at 12:38:01PM +0000, Richard Sandiford wrote:
> Peter Bergner <bergner@linux.ibm.com> writes:
> > Adding Richard since he's reviewed the generic opaque mode code in
> > the past and this patch contains some more eneric support.
> >
> > GCC handles pseudos that are used uninitialized, by emitting a
> > (set (reg: <reg>) CONST0_RTX(regmode)) before their uninitialized
> > pseudo usage.  Currently, CONST0_RTX(mode) is NULL for opaque modes,
> > which leads to an ICE.  The solution is to enhance init_emit_once() to
> > add initialization of CONST0_RTX for opaque modes using a target hook,
> > since CONST0_RTX probably are different for each opaque mode and each
> > target.  The default hook throws an error to force the target to think
> > hard about what their CONST0_RTX values should be for each mode.
> 
> Hmm, but it looks like the hook uses const0_rtx, i.e. a const_int,
> for something that isn't an integer mode.  Also, the unspec for XOmode
> isn't a constant in the normal sense (CONSTANT_P).
> 
> I think we should either add a new rtx code for constant opaque modes
> or make init-regs just emit the clobber for opaque modes (and not emit
> the move).

Thanks for looking Richard.  That last option sounds good to me as well.

Some comments on the patch:

> > --- a/gcc/config/rs6000/mma.md
> > +++ b/gcc/config/rs6000/mma.md
> > @@ -473,9 +473,7 @@

Please look at the commentary in $GCCSRC/.gitattributes on how to get
context shown in .md diffs (it needs a local configuration step).

> > @@ -16624,6 +16628,19 @@ rs6000_split_multireg_move (rtx dst, rtx src)
> > +      /* Split the clearing of an OOmode register pair into clearing
> > +	 of its two constituent registers.  */
> > +      if (REG_P (dst) && mode == OOmode && src == CONST0_RTX (mode))
> > +	{
> > +	  int regno = REGNO (dst);
> > +	  gcc_assert (VSX_REGNO_P (regno));
> > +	  emit_insn (gen_rtx_SET (gen_rtx_REG (reg_mode, regno),
> > +				  CONST0_RTX (reg_mode)));
> > +	  emit_insn (gen_rtx_SET (gen_rtx_REG (reg_mode, regno + 1),
> > +				  CONST0_RTX (reg_mode)));
> > +	  return;
> > +	}

That is fine.

> > +/* Implement TARGET_OPAQUE_CONST0_RTX.  */
> > +
> > +rtx
> > +rs6000_opaque_const0_rtx (machine_mode mode)
> > +{
> > +  gcc_assert (OPAQUE_MODE_P (mode));
> > +
> > +  switch (mode)
> > +    {
> > +    case E_OOmode:
> > +      return const0_rtx;
> > +    case E_XOmode:
> > +      return gen_rtx_UNSPEC (XOmode, gen_rtvec (1, const0_rtx),
> > +			     UNSPEC_MMA_XXSETACCZ);
> > +    default:
> > +      gcc_unreachable ();
> > +    }
> > +}

Why are XO and OO handled in different ways?  This needs a comment, or
better, not be handled in different ways ;-)

> > --- a/gcc/emit-rtl.c
> > +++ b/gcc/emit-rtl.c
> > @@ -6408,6 +6408,11 @@ init_emit_once (void)
> >      if (GET_MODE_CLASS ((machine_mode) i) == MODE_CC)
> >        const_tiny_rtx[0][i] = const0_rtx;
> >  
> > +  FOR_EACH_MODE_IN_CLASS (mode, MODE_OPAQUE)
> > +    {
> > +      const_tiny_rtx[0][(int) mode] = targetm.opaque_const0_rtx (mode);
> > +    }

This does not sound like a good idea, it is asking for trouble imo.

> > +rtx
> > +hook_rtx_mode_unreachable (machine_mode mode ATTRIBUTE_UNUSED)
> > +{
> > +  gcc_unreachable ();
> > +}

If you want to use this in any hook, that hook needs documentation that
it can not be used on some targets (an ICE is not acceptable from a UI
point of view, in almost all cases).

> > +DEFHOOK
> > +(opaque_const0_rtx,
> > + "Return an RTX representing the value @code{0} for opaque mode @var{mode}.\n\
> > +The default version of this hook always throws an error.",

So that documentation needs a severe warning in it.

> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr98872.c
> > @@ -0,0 +1,20 @@
> > +/* PR target/98872 */
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target power10_ok } */
> > +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> > +
> > +/* Verify we do not ICE on the tests below.  */

Do the existing tests already check the expected code for this?


I would expect the code that initialises uninitialised values to handle
this, instead (possibly call the same hook, but :-) )


Segher