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

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

[Ajit Agarwal]

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;
+}
+
+/* Return TRUE if the candidate insn doesn't have defs and have
+ * uses without RTX_BIN_ARITH/RTX_COMM_ARITH/RTX_UNARY rtx class.  */
+
+static bool
+abi_handle_regs_without_defs_p (rtx_insn *insn)
+{
+  if (side_effects_p (PATTERN (insn)))
+    return false;
+
+  struct df_link *uses
+    = get_uses (insn, SET_DEST (PATTERN (insn)));
+
+  if (!uses)
+    return false;
+
+  for (df_link *use = uses; use; use = use->next)
+    {
+      if (!use->ref)
+	return false;
+
+      if (BLOCK_FOR_INSN (insn)
+	  != BLOCK_FOR_INSN (DF_REF_INSN (use->ref)))
+	return false;
+
+      rtx_insn *use_insn = DF_REF_INSN (use->ref);
+
+      if (GET_CODE (PATTERN (use_insn)) == SET)
+	{
+	  rtx_code code = GET_CODE (SET_SRC (PATTERN (use_insn)));
+
+	  if (GET_RTX_CLASS (code) == RTX_BIN_ARITH
+	      || GET_RTX_CLASS (code) == RTX_COMM_ARITH
+	      || GET_RTX_CLASS (code) == RTX_UNARY)
+	    return false;
+	}
+     }
+  return true;
+}
+
 /* This function goes through all reaching defs of the source
    of the candidate for elimination (CAND) and tries to combine
    the extension with the definition instruction.  The changes
@@ -770,6 +869,11 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
 
   state->defs_list.truncate (0);
   state->copies_list.truncate (0);
+  rtx orig_src = XEXP (SET_SRC (cand->expr),0);
+
+  if (abi_extension_candidate_p (cand->insn)
+      && (!get_defs (cand->insn, orig_src, NULL)))
+    return abi_handle_regs_without_defs_p (cand->insn);
 
   outcome = make_defs_and_copies_lists (cand->insn, set_pat, state);
 
@@ -1112,26 +1216,34 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
       rtx reg = XEXP (src, 0);
       struct df_link *defs, *def;
       ext_cand *cand;
+      defs = get_defs (insn, reg, NULL);
 
       /* Zero-extension of an undefined value is partly defined (it's
 	 completely undefined for sign-extension, though).  So if there exists
 	 a path from the entry to this zero-extension that leaves this register
 	 uninitialized, removing the extension could change the behavior of
 	 correct programs.  So first, check it is not the case.  */
-      if (code == ZERO_EXTEND && !bitmap_bit_p (init_regs, REGNO (reg)))
+      if (!defs && abi_extension_candidate_argno_p (code, REGNO (reg)))
 	{
-	  if (dump_file)
-	    {
-	      fprintf (dump_file, "Cannot eliminate extension:\n");
-	      print_rtl_single (dump_file, insn);
-	      fprintf (dump_file, " because it can operate on uninitialized"
-			          " data\n");
-	    }
+	  ext_cand e = {expr, code, mode, insn};
+	  insn_list->safe_push (e);
 	  return;
 	}
 
+       if ((code == ZERO_EXTEND
+	    && !bitmap_bit_p (init_regs, REGNO (reg))))
+	  {
+	    if (dump_file)
+	      {
+		fprintf (dump_file, "Cannot eliminate extension:\n");
+		print_rtl_single (dump_file, insn);
+		fprintf (dump_file, " because it can operate on uninitialized"
+				    " data\n");
+	      }
+	    return;
+	  }
+
       /* Second, make sure we can get all the reaching definitions.  */
-      defs = get_defs (insn, reg, NULL);
       if (!defs)
 	{
 	  if (dump_file)
@@ -1321,7 +1433,8 @@ find_and_remove_re (void)
 	      && (REGNO (SET_DEST (set)) != REGNO (XEXP (SET_SRC (set), 0))))
 	    {
               reinsn_copy_list.safe_push (curr_cand->insn);
-              reinsn_copy_list.safe_push (state.defs_list[0]);
+	      if (state.defs_list.length() != 0)
+		reinsn_copy_list.safe_push (state.defs_list[0]);
 	    }
 	  reinsn_del_list.safe_push (curr_cand->insn);
 	  state.modified[INSN_UID (curr_cand->insn)].deleted = 1;
@@ -1342,24 +1455,27 @@ find_and_remove_re (void)
      Remember that the memory reference will be changed to refer to the
      destination of the extention.  So we're actually emitting a copy
      from the new destination to the old destination.  */
-  for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
-    {
-      rtx_insn *curr_insn = reinsn_copy_list[i];
-      rtx_insn *def_insn = reinsn_copy_list[i + 1];
-
-      /* Use the mode of the destination of the defining insn
-	 for the mode of the copy.  This is necessary if the
-	 defining insn was used to eliminate a second extension
-	 that was wider than the first.  */
-      rtx sub_rtx = *get_sub_rtx (def_insn);
-      rtx set = single_set (curr_insn);
-      rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
-				 REGNO (XEXP (SET_SRC (set), 0)));
-      rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
-				 REGNO (SET_DEST (set)));
-      rtx new_set = gen_rtx_SET (new_dst, new_src);
-      emit_insn_after (new_set, def_insn);
-    }
+   for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
+     {
+       rtx_insn *curr_insn = reinsn_copy_list[i];
+
+       if ((i+1) < reinsn_copy_list.length())
+	 {
+	   rtx_insn *def_insn = reinsn_copy_list[i + 1];
+	   /* Use the mode of the destination of the defining insn
+	      for the mode of the copy.  This is necessary if the
+	      defining insn was used to eliminate a second extension
+	      that was wider than the first.  */
+	   rtx sub_rtx = *get_sub_rtx (def_insn);
+	   rtx set = single_set (curr_insn);
+	   rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
+				      REGNO (XEXP (SET_SRC (set), 0)));
+	   rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
+				      REGNO (SET_DEST (set)));
+	   rtx new_set = gen_rtx_SET (new_dst, new_src);
+	   emit_insn_after (new_set, def_insn);
+	}
+      }
 
   /* Delete all useless extensions here in one sweep.  */
   FOR_EACH_VEC_ELT (reinsn_del_list, i, curr_insn)
diff --git a/gcc/testsuite/g++.target/powerpc/zext-elim-3.C b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
new file mode 100644
index 00000000000..1d443af066a
--- /dev/null
+++ b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
@@ -0,0 +1,16 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-require-effective-target lp64 } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mcpu=power9 -O2 -free" } */
+
+void *memset(void *b, int c, unsigned long len)
+{
+  unsigned long i;
+
+  for (i = 0; i < len; i++)
+    ((unsigned char *)b)[i] = c;
+
+   return b;
+}
+
+/* { dg-final { scan-assembler-not "rlwinm" } } */
-- 
2.31.1

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

[Hans-Peter Nilsson]

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?

Also, "!=  ZERO_EXTEND" has too many spaces, copy-pasted in 
several (all?) places.

Also, s/an return  registers/a return register/ (three errors).

brgds, H-P

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

[Jeff Law]

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

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

[Hans-Peter Nilsson]

On Fri, 28 Apr 2023, Jeff Law wrote:
> On 4/28/23 16:42, Hans-Peter Nilsson wrote:
> > On Sat, 22 Apr 2023, Ajit Agarwal via Gcc-patches wrote:
> > 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.

Right, I thought this was the new version.

> 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.

IIRC I also commented and suggested a few target macros that 
*should* have helped to that effect.  Ajit, I suggest you see my 
previous reply in this or a related conversation.

brgds, H-P

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

[Peter Bergner]

On 4/28/23 6:49 PM, Hans-Peter Nilsson wrote:
> On Fri, 28 Apr 2023, Jeff Law wrote:
>> 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.
> 
> IIRC I also commented and suggested a few target macros that 
> *should* have helped to that effect.  Ajit, I suggest you see my 
> previous reply in this or a related conversation.

To be specific, Hans Peter said earlier:


On 3/30/23 7:01 PM, Hans-Peter Nilsson wrote:
> Pardon the arm-chair development mode but it sounds like 
> re-inventing the TARGET_PROMOTE_* hooks...
> 
> Maybe just hook up TARGET_PROMOTE_FUNCTION_MODE to ree.c (as 
> "you" already already define it for "rs6000")?


Peter

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

[Ajit Agarwal]

On 29/04/23 5:03 am, Jeff Law wrote:
> 
> 
> 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.


The ABI we need for ree pass are the argument registers and return registers. Based on that I have described interfaces that we need. Other than that we dont any other ABI hooks. I have used FUNCTION_VALUE_REGNO_P and FuNCTION_ARG_REGNO_P abi hooks.

Thanks & Regards
Ajit
> 
> Jeff

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

[Jeff Law]

On 5/16/23 06:35, Ajit Agarwal wrote:
> 
> 
> On 29/04/23 5:03 am, Jeff Law wrote:
>>
>>
>> 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.
> 
> 
> The ABI we need for ree pass are the argument registers and return registers. Based on that I have described interfaces that we need. Other than that we dont any other ABI hooks. I have used FUNCTION_VALUE_REGNO_P and FuNCTION_ARG_REGNO_P abi hooks.
You're working with one of many ABIs, some of which have useful 
properties, some of which do not.

Simply testing FUNCTION_VALUE_REGNO_P/FUNCTION_ARG_REGNO_P is not 
sufficient.  You need to be able to query the ABI properties.

jeff

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

[Segher Boessenkool]

Hi!

On Sat, Apr 22, 2023 at 02:36:20PM +0530, Ajit Agarwal wrote:
>         * ree.cc (combline_reaching_defs): Add zero_extend
>         using defined abi interfaces.

Typo.  Also, please don't wrap lines early.  Also, you are missing some
changes in this file in the changelog.

>         (add_removable_extension): use of defined abi interfaces
>         for no reaching defs.

Capital U.

>         (abi_extension_candidate_return_reg_p): New defined ABI function.

What does that even mean?  An "ABI function"?

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

If it is suddenly allowed to have nil here, some comment somewhere needs
to change as well.

> new file mode 100644
> index 00000000000..1d443af066a
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
> @@ -0,0 +1,16 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */

This is required of all file in g++.target/powerpc/ already:
# Exit immediately if this isn't a PowerPC target.
if {![istarget powerpc*-*-*] } then {
  return
}

so please don't repeat that here.

> +/* { dg-require-effective-target lp64 } */

Is that required?!

> +/* { dg-require-effective-target powerpc_p9vector_ok } */
> +/* { dg-options "-mcpu=power9 -O2 -free" } */

Why does this need p9?  We should test this on older systems as well, it
is a problem as old as the world!

> +void *memset(void *b, int c, unsigned long len)
> +{
> +  unsigned long i;
> +
> +  for (i = 0; i < len; i++)
> +    ((unsigned char *)b)[i] = c;
> +
> +   return b;
> +}
> +
> +/* { dg-final { scan-assembler-not "rlwinm" } } */

Please at least use {\mrlwinm\M}.  There are many other insns that can
be used for this same purpose as well.  We should at least test rldicl
here as well.


Segher