[1/3] rs6000: update num_insns_constant for 2 insns

  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.

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(-)
  

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);
  

Hi,

"Kewen.Lin" <linkw@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);