[RS6000] push_secondary_reload ICE

target.h struct secondary_reload_info has two fields that "are for the
use of the backward compatibility hook", default_secondary_reload.
t_icode is used to pass information from one invocation of
default_secondary_reload to the next, between the
targetm.secondary_reload call below in push_secondary_reload to the
same call inside the recursive call to push_secondary_reload a few
lines later.

  rclass = (enum reg_class) targetm.secondary_reload (in_p, x, reload_class,
						      reload_mode, &sri);
  icode = (enum insn_code) sri.icode;

  /* If we don't need any secondary registers, done.  */
  if (rclass == NO_REGS && icode == CODE_FOR_nothing)
    return -1;

  if (rclass != NO_REGS)
    t_reload = push_secondary_reload (in_p, x, opnum, optional, rclass,
				      reload_mode, type, &t_icode, &sri);

If a target secondary_reload hook ever calls default_secondary_reload
on a recursive call to push_secondary_reload, but does not call
default_secondary_reload on the first call, then we have an
uninitialized t_icode field.

The rs6000 backend was doing exactly this on the pr72103 testcase.

This insn
(insn 488 206 406 31 (set (reg:DI 317)
        (unspec:DI [
                (fix:SI (reg:DF 105 28 [orig:173 _34 ] [173]))
            ] UNSPEC_FCTIWZ)) /home/alan/src/tmp/pr72103.ii:47 373 {fctiwz_df}
     (nil))
didn't get a hard reg for reg:DI 317, and ira decided (reasonably)
that reg 317 ought to be in class VSX_REGS.  When storing reg 317 to
its stack slot, the following code in rs6000_secondary_reload
triggered.

  /* If this is a scalar floating point value and we want to load it into the
     traditional Altivec registers, do it via a move via a traditional floating
     point register, unless we have D-form addressing.  Also make sure that
     non-zero constants use a FPR.  */
  if (!done_p && reg_addr[mode].scalar_in_vmx_p
      && !mode_supports_vmx_dform (mode)
      && (rclass == VSX_REGS || rclass == ALTIVEC_REGS)
      && (memory_p || (GET_CODE (x) == CONST_DOUBLE)))
    {
      ret = FLOAT_REGS;
      default_p = false;
      done_p = true;
    }

For the pr72103 testcase this is effectively saying we shouldn't store
a DImode vsx reg to a stack slot directly, but instead should reload
into a float reg.  Which seems a little odd to me..

Anyway, regardless of whether this PR needs more attention, I believe
we should stop using default_secondary_reload or ensure it isn't
presented with uninitialized data.  The following cures the ICE and
has been bootstrapped and regression tested powerpc64le-linux.

OK for mainline?

	PR target/72103
	* config/rs6000/rs6000.c (rs6000_secondary_reload): Initialize
	sri->t_icode.

Hi Alan,

Thanks for the writeup.

On Tue, Jul 26, 2016 at 04:09:12PM +0930, Alan Modra wrote:
>   /* If this is a scalar floating point value and we want to load it into the
>      traditional Altivec registers, do it via a move via a traditional floating
>      point register, unless we have D-form addressing.  Also make sure that
>      non-zero constants use a FPR.  */
>   if (!done_p && reg_addr[mode].scalar_in_vmx_p
>       && !mode_supports_vmx_dform (mode)
>       && (rclass == VSX_REGS || rclass == ALTIVEC_REGS)
>       && (memory_p || (GET_CODE (x) == CONST_DOUBLE)))
>     {
>       ret = FLOAT_REGS;
>       default_p = false;
>       done_p = true;
>     }
> 
> For the pr72103 testcase this is effectively saying we shouldn't store
> a DImode vsx reg to a stack slot directly, but instead should reload
> into a float reg.  Which seems a little odd to me..

I added SCALAR_FLOAT_MODE_P (mode) to the condition, which also fixes
this particular PR, and makes this do what the comment says it does.
Mike?  What is best here?  Change the comment or change the code?

> Anyway, regardless of whether this PR needs more attention, I believe
> we should stop using default_secondary_reload or ensure it isn't
> presented with uninitialized data.  The following cures the ICE and
> has been bootstrapped and regression tested powerpc64le-linux.
> 
> OK for mainline?

Yes please.  It is also okay for backports (it seems to be needed on 5
and 6, although the testcase in PR72103 does not fail there).

Thanks,


Segher

> 	PR target/72103
> 	* config/rs6000/rs6000.c (rs6000_secondary_reload): Initialize
> 	sri->t_icode.

On Tue, Jul 26, 2016 at 03:26:55AM -0500, Segher Boessenkool wrote:
> On Tue, Jul 26, 2016 at 04:09:12PM +0930, Alan Modra wrote:
> >   /* If this is a scalar floating point value and we want to load it into the
> >      traditional Altivec registers, do it via a move via a traditional floating
> >      point register, unless we have D-form addressing.  Also make sure that
> >      non-zero constants use a FPR.  */
> >   if (!done_p && reg_addr[mode].scalar_in_vmx_p
> >       && !mode_supports_vmx_dform (mode)
> >       && (rclass == VSX_REGS || rclass == ALTIVEC_REGS)
> >       && (memory_p || (GET_CODE (x) == CONST_DOUBLE)))
> >     {
> >       ret = FLOAT_REGS;
> >       default_p = false;
> >       done_p = true;
> >     }
> > 
> > For the pr72103 testcase this is effectively saying we shouldn't store
> > a DImode vsx reg to a stack slot directly, but instead should reload
> > into a float reg.  Which seems a little odd to me..
> 
> I added SCALAR_FLOAT_MODE_P (mode) to the condition, which also fixes
> this particular PR, and makes this do what the comment says it does.
> Mike?  What is best here?  Change the comment or change the code?

Also, the comment only mentions loads whereas the code affects both
loads and stores.