Hi,
on 2023/10/25 10:00, Jiufu Guo wrote:
> Hi,
>
> Trunk gcc supports more constants to be built via two instructions: e.g.
> "li/lis; xori/xoris/rldicl/rldicr/rldic".
> And then num_insns_constant should also be updated.
>
Thanks for updating this.
> Bootstrap & regtest pass ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu Guo)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_built_by_lilis_and_rldicX): New
> function.
> (num_insns_constant_gpr): Update to return 2 for more cases.
> (rs6000_emit_set_long_const): Update to use
> can_be_built_by_lilis_and_rldicX.
>
> ---
> gcc/config/rs6000/rs6000.cc | 64 ++++++++++++++++++++++++-------------
> 1 file changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index cc24dd5301e..b23ff3d7917 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -6032,6 +6032,9 @@ direct_return (void)
> return 0;
> }
>
> +static bool
> +can_be_built_by_lilis_and_rldicX (HOST_WIDE_INT, int *, HOST_WIDE_INT *);
> +
> /* Helper for num_insns_constant. Calculate number of instructions to
> load VALUE to a single gpr using combinations of addi, addis, ori,
> oris, sldi and rldimi instructions. */
> @@ -6044,35 +6047,41 @@ num_insns_constant_gpr (HOST_WIDE_INT value)
> return 1;
>
> /* constant loadable with addis */
> - else if ((value & 0xffff) == 0
> - && (value >> 31 == -1 || value >> 31 == 0))
> + if ((value & 0xffff) == 0 && (value >> 31 == -1 || value >> 31 == 0))
> return 1;
>
> /* PADDI can support up to 34 bit signed integers. */
> - else if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
> + if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
> return 1;
>
> - else if (TARGET_POWERPC64)
> - {
> - HOST_WIDE_INT low = sext_hwi (value, 32);
> - HOST_WIDE_INT high = value >> 31;
> + if (!TARGET_POWERPC64)
> + return 2;
>
> - if (high == 0 || high == -1)
> - return 2;
> + HOST_WIDE_INT low = sext_hwi (value, 32);
> + HOST_WIDE_INT high = value >> 31;
>
> - high >>= 1;
> + if (high == 0 || high == -1)
> + return 2;
>
> - if (low == 0 || low == high)
> - return num_insns_constant_gpr (high) + 1;
> - else if (high == 0)
> - return num_insns_constant_gpr (low) + 1;
> - else
> - return (num_insns_constant_gpr (high)
> - + num_insns_constant_gpr (low) + 1);
> - }
> + high >>= 1;
>
> - else
> + HOST_WIDE_INT ud2 = (low >> 16) & 0xffff;
> + HOST_WIDE_INT ud1 = low & 0xffff;
> + if (high == -1 && ((!(ud2 & 0x8000) && ud1 == 0) || (ud1 & 0x8000)))
> + return 2;
> + if (high == 0 && (ud1 == 0 || (!(ud1 & 0x8000))))
> return 2;
I was thinking that instead of enumerating all the cases in function
rs6000_emit_set_long_const, if we can add one optional argument like
"int* num_insns=nullptr" to function rs6000_emit_set_long_const, and
when it's not nullptr, not emitting anything but update the count in
rs6000_emit_set_long_const. It helps people remember to update
num_insns when updating rs6000_emit_set_long_const in future, it's
also more clear on how the number comes from.
Does it sound good to you?
BR,
Kewen
> +
> + int shift;
> + HOST_WIDE_INT mask;
> + if (can_be_built_by_lilis_and_rldicX (value, &shift, &mask))
> + return 2;
> +
> + if (low == 0 || low == high)
> + return num_insns_constant_gpr (high) + 1;
> + if (high == 0)
> + return num_insns_constant_gpr (low) + 1;
> + return (num_insns_constant_gpr (high) + num_insns_constant_gpr (low) + 1);
> }
>
> /* Helper for num_insns_constant. Allow constants formed by the
> @@ -10492,6 +10501,18 @@ can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int
> *shift, HOST_WIDE_INT *mask)
> return false;
> }
>
> +/* Combine the above checking functions for li/lis;rldicX. */
> +
> +static bool
> +can_be_built_by_lilis_and_rldicX (HOST_WIDE_INT c, int *shift,
> + HOST_WIDE_INT *mask)
> +{
> + return (can_be_built_by_li_lis_and_rotldi (c, shift, mask)
> + || can_be_built_by_li_lis_and_rldicl (c, shift, mask)
> + || can_be_built_by_li_lis_and_rldicr (c, shift, mask)
> + || can_be_built_by_li_and_rldic (c, shift, mask));
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10538,10 +10559,7 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT
> c)
> emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> GEN_INT ((ud2 ^ 0xffff) << 16)));
> }
> - else if (can_be_built_by_li_lis_and_rotldi (c, &shift, &mask)
> - || can_be_built_by_li_lis_and_rldicl (c, &shift, &mask)
> - || can_be_built_by_li_lis_and_rldicr (c, &shift, &mask)
> - || can_be_built_by_li_and_rldic (c, &shift, &mask))
> + else if (can_be_built_by_lilis_and_rldicX (c, &shift, &mask))
> {
> temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> unsigned HOST_WIDE_INT imm = (c | ~mask);
