Re: [PATCH v4 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces.

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/28/23 16:42, Hans-Peter Nilsson wrote:
> On Sat, 22 Apr 2023, Ajit Agarwal via Gcc-patches wrote:
> 
>> Hello All:
>>
>> This new version of patch 4 use improve ree pass for rs6000 target using defined ABI interfaces.
>> Bootstrapped and regtested on power64-linux-gnu.
>>
>> Thanks & Regards
>> Ajit
>>
>>
>> 	ree: Improve ree pass for rs6000 target using defined abi interfaces
>>
>>          For rs6000 target we see redundant zero and sign
>>          extension and done to improve ree pass to eliminate
>>          such redundant zero and sign extension using defines
>>          ABI interfaces.
>>
>>          2023-04-22  Ajit Kumar Agarwal  <aagarwa1@linux.ibm.com>
>>
>> gcc/ChangeLog:
>>
>>          * ree.cc (combline_reaching_defs): Add zero_extend
>>          using defined abi interfaces.
>>          (add_removable_extension): use of defined abi interfaces
>>          for no reaching defs.
>>          (abi_extension_candidate_return_reg_p): New defined ABI function.
>>          (abi_extension_candidate_p): New defined ABI function.
>>          (abi_extension_candidate_argno_p): New defined ABI function.
>>          (abi_handle_regs_without_defs_p): New defined ABI function.
>>
>> gcc/testsuite/ChangeLog:
>>
>>          * g++.target/powerpc/zext-elim-3.C
>> ---
>>   gcc/ree.cc                                    | 176 +++++++++++++++---
>>   .../g++.target/powerpc/zext-elim-3.C          |  16 ++
>>   2 files changed, 162 insertions(+), 30 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim-3.C
>>
>> diff --git a/gcc/ree.cc b/gcc/ree.cc
>> index 413aec7c8eb..0de96b1ece1 100644
>> --- a/gcc/ree.cc
>> +++ b/gcc/ree.cc
>> @@ -473,7 +473,8 @@ get_defs (rtx_insn *insn, rtx reg, vec<rtx_insn *> *dest)
>>   	break;
>>       }
>>   
>> -  gcc_assert (use != NULL);
>> +  if (use == NULL)
>> +    return NULL;
>>   
>>     ref_chain = DF_REF_CHAIN (use);
>>   
>> @@ -514,7 +515,8 @@ get_uses (rtx_insn *insn, rtx reg)
>>       if (REGNO (DF_REF_REG (def)) == REGNO (reg))
>>         break;
>>   
>> -  gcc_assert (def != NULL);
>> +  if (def == NULL)
>> +    return NULL;
>>   
>>     ref_chain = DF_REF_CHAIN (def);
>>   
>> @@ -750,6 +752,103 @@ get_extended_src_reg (rtx src)
>>     return src;
>>   }
>>   
>> +/* Return TRUE if the candidate insn is zero extend and regno is
>> +   an return  registers.  */
>> +
>> +static bool
>> +abi_extension_candidate_return_reg_p (rtx_insn *insn, int regno)
>> +{
>> +  rtx set = single_set (insn);
>> +
>> +  if (GET_CODE (SET_SRC (set)) !=  ZERO_EXTEND)
>> +    return false;
>> +
>> +  if (FUNCTION_VALUE_REGNO_P (regno))
>> +    return true;
>> +
>> +  return false;
>> +}
>> +
>> +/* Return TRUE if reg source operand of zero_extend is argument registers
>> +   and not return registers and source and destination operand are same
>> +   and mode of source and destination operand are not same.  */
>> +
>> +static bool
>> +abi_extension_candidate_p (rtx_insn *insn)
>> +{
>> +  rtx set = single_set (insn);
>> +
>> +  if (GET_CODE (SET_SRC (set)) !=  ZERO_EXTEND)
>> +    return false;
>> +
>> +  machine_mode ext_dst_mode = GET_MODE (SET_DEST (set));
>> +  rtx orig_src = XEXP (SET_SRC (set),0);
>> +
>> +  bool copy_needed
>> +    = (REGNO (SET_DEST (set)) != REGNO (XEXP (SET_SRC (set), 0)));
>> +
>> +  if (!copy_needed && ext_dst_mode != GET_MODE (orig_src)
>> +      && FUNCTION_ARG_REGNO_P (REGNO (orig_src))
>> +      && !abi_extension_candidate_return_reg_p (insn, REGNO (orig_src)))
>> +    return true;
>> +
>> +  return false;
>> +}
>> +
>> +/* Return TRUE if the candidate insn is zero extend and regno is
>> +   an argument registers.  */
>> +
>> +static bool
>> +abi_extension_candidate_argno_p (rtx_code code, int regno)
>> +{
>> +  if (code !=  ZERO_EXTEND)
>> +    return false;
>> +
>> +  if (FUNCTION_ARG_REGNO_P (regno))
>> +    return true;
>> +
>> +  return false;
>> +}
> 
> I don't see anything in those functions that checks if
> ZERO_EXTEND is actually a feature of the ABI, e.g. as opposed to
> no extension or SIGN_EXTEND.  Do I miss something?
I don't think you missed anything.  That was one of the points I was 
making last week.  Somewhere, somehow we need to describe what the ABI 
mandates and guarantees.

So while what Ajit has done is a step forward, at some point the actual 
details of the ABI need to be described in a way that can be checked and 
consumed by REE.

Jeff