Hi Richard,

Here's the updated patch, thanks for the feedback so far!

Regards,
Tamar
________________________________________
From: Richard Sandiford <richard.sandiford@linaro.org>
Sent: Thursday, June 8, 2017 11:32:07 AM
To: Tamar Christina
Cc: GCC Patches; nd; James Greenhalgh; Marcus Shawcroft; Richard Earnshaw
Subject: Re: [PATCH][GCC][AArch64] optimize float immediate moves (1 /4) - infrastructure.

Tamar Christina <Tamar.Christina@arm.com> writes:
> @@ -4613,6 +4615,66 @@ aarch64_legitimize_address_displacement (rtx *disp, rtx *off, machine_mode mode)
>    return true;
>  }
>
> +/* Return the binary representation of floating point constant VALUE in INTVAL.
> +   If the value cannot be converted, return false without setting INTVAL.
> +   The conversion is done in the given MODE.  */
> +bool
> +aarch64_reinterpret_float_as_int (rtx value, unsigned HOST_WIDE_INT *intval)
> +{
> +  machine_mode mode = GET_MODE (value);
> +  if (GET_CODE (value) != CONST_DOUBLE
> +      || !SCALAR_FLOAT_MODE_P (mode)
> +      || GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
> +    return false;
> +
> +  unsigned HOST_WIDE_INT ival = 0;
> +
> +  /* Only support up to DF mode.  */
> +  gcc_assert (GET_MODE_BITSIZE (mode) <= 64);
> +  int needed = GET_MODE_BITSIZE (mode) == 64 ? 2 : 1;
> +
> +  long res[2];
> +  real_to_target (res,
> +               CONST_DOUBLE_REAL_VALUE (value),
> +               REAL_MODE_FORMAT (mode));
> +
> +  ival = zext_hwi (res[needed - 1], 32);
> +  for (int i = needed - 2; i >= 0; i--)
> +    {
> +      ival <<= 32;
> +      ival |= zext_hwi (res[i], 32);
> +    }
> +
> +  *intval = ival;
> +  return true;
> +}
> +
> +/* Return TRUE if rtx X is an immediate constant that can be moved in
> +   created using a single MOV(+MOVK) followed by an FMOV.  */

Typo.

> +bool
> +aarch64_float_const_rtx_p (rtx x)
> +{
> +  machine_mode mode = GET_MODE (x);
> +  if (mode == VOIDmode)
> +    return false;
> +
> +  /* Determine whether it's cheaper to write float constants as
> +     mov/movk pairs over ldr/adrp pairs.  */
> +  unsigned HOST_WIDE_INT ival;
> +
> +  if (GET_CODE (x) == CONST_DOUBLE
> +      && SCALAR_FLOAT_MODE_P (mode)
> +      && aarch64_reinterpret_float_as_int (x, &ival))
> +    {
> +      machine_mode imode = mode == HFmode ? SImode : int_mode_for_mode (mode);
> +      int num_instr = aarch64_internal_mov_immediate
> +                     (NULL_RTX, GEN_INT (ival), false, imode);

Sorry to be a broken record on this, but since you explicitly zero-extend
the real_to_target results from 32 bits, this will lead to an invalid
32-bit constant when the top bit of an SImode value is set,
e.g. (const_int 0x8000_0000) instead of (const_int 0xffff_ffff_8000_0000).

Using gen_int_mode would avoid this.  In general it's better to use
gen_int_mode instead of GEN_INT whenever the mode is known, to avoid
this kind of corner case.

There's another instance later in the patch.

Thanks,
Richard