Hi!
On Mon, Feb 08, 2021 at 12:38:01PM +0000, Richard Sandiford wrote:
> Peter Bergner <[email protected]> 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
