Hi!
On Sun, Nov 25, 2018 at 10:49:12PM +1030, Alan Modra wrote:
> This patch came about from investigating an ICE that appeared when I
> was retesting an old half-baked patch of mine to rs6000_rtx_costs.
> If a const_double is fed to rs6000_is_valid_and_mask and from there to
> rs6000_is_valid_mask where INTVAL is used, gcc will ICE.
>
> The num_insns_constant ICE was introduced with git commit f337168d97.
> However, the code was buggy before that. There was no point in
> testing for a mask since the mask predicates only handle const_int.
> In fact, I don't think the function ever handled floating point
> constants that might match a load of minus one and mask. It does now.
> I've added a few comments regarding splitters so the next person
> looking at this code can see how this works.
>
> The patch also extracts code out of num_insns_constant that needed to
> handle multiple gprs for DFmode constants in 32-bit mode, to a
> function that handles multiple gprs a little more generally. I don't
> think there is any need for anything but the 32-bit DFmode case
> currently, but this allows for possible future uses. The
> CONST_WIDE_INT case is also not used currently, and needed fixing.
> Adding CONST_WIDE_INT_NUNITS - 1 only makes sense if the elements of
> the array were being shifted into a register of size larger than the
> element size (which is 64-bits).
>
> Boostrapped etc. powerpc64le-linux and powerpc64-linux.
>
> * config/rs6000/rs6000.c (num_insns_constant_gpr): Renamed from
> num_insns_constant_wide. Make static. Revise comment.
> (num_insns_constant_multi): New function.
> (num_insns_constant): Formatting. Correct CONST_WIDE_INT
> calculation. Simplify and extract code common to both
> CONST_INT and CONST_DOUBLE. Add gcc_unreachable for unhandled
> const_double modes.
> * config/rs6000/rs6000-protos.h (num_insns_const_wide): Delete.
> +/* Helper for num_insn_constant. Allow constants formed by the
> + num_insns_constant_gpr sequences, plus li -1, rldicl/rldicr/rlwinm,
> + and handle modes that require multiple gprs. */
> +
> +static int
> +num_insns_constant_multi (HOST_WIDE_INT value, machine_mode mode)
> +{
> + int nregs = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
> + int total = 0;
> + while (nregs-- > 0)
> + {
> + int ins = num_insns_constant_gpr (value);
You probably should mask "value" here so that it is only one GPR.
Alternatively, make num_insns_constant_gpr handle that. Consider what
happens for a 64-bit number with 32-bit registers here?
Also, s/ins/insns/ please.
> + if (ins > 2
> + /* We won't get more than 2 from num_insns_constant_gpr
> + except when TARGET_POWERPC64 and mode is DImode or
> + wider, so the register mode must be DImode. */
> + && rs6000_is_valid_and_mask (GEN_INT (value), DImode))
> + ins = 2;
> + total += ins;
> + value >>= 32;
> + if (TARGET_POWERPC64)
> + value >>= 32;
That's just
value >>= BITS_PER_WORD;
> + long l[2];
> + val = l[WORDS_BIG_ENDIAN ? 0 : 1] << 32;
This doesn't work, as in the other patch: long can be 32 bit.
Looks good otherwise.
Segher
