Hi,
"Kewen.Lin" <li...@linux.ibm.com> writes:
> 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?
Thanks for your advice! Yes, "rs6000_emit_set_long_const" and
"num_insns_constant_gpr" should be aligned. Using a same logic
(same code place) would make sense.
BR,
Jeff (Jiufu Guo)
>
> 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);
