Re: [PATCH] Improve prepare_shrink_wrap to sink more instructions

[Jiong Wang <jiong dot wang at arm dot com>]

On 11/09/14 21:39, Jeff Law wrote:
> On 09/08/14 07:57, Jiong Wang wrote:
> > > Conceptually OK.  Some questions/concerns about the implementation.
> > Hi Jeff,
> >
> >     thanks very much for your review.
> > > It seems to me that what you're trying to describe on the RHS is
> > > REG_P || CONSTANT_P
> >     yes.
> >
> >     and actually I am trying to detect all rtx which contains any number
> >     of RTX_CONST_OBJs and no more than one REG.
> > > Note that CONSTANT_P will catch things like (high (symbol_ref)) and
> > > (lo_sum (reg) (symbol_ref)) which are often used to build addresses on
> > > some targets.
> > >
> > > With that in mind, rather than using a for_each_rtx callback, test
> > > if (REG_P (src) || CONSTANT_P (src))
> > >
> > > Note that SRC, when it is a CONSTANT_P,  may have a variety of forms
> > > with embedded registers.  You'll need to verify that all of those
> > > registers are not assigned after the insn with the CONSTANT_P source
> > > operand.  Right now you only perform that check when SRC is a REG_P.
> >     I am using the for_each_rtx because I want to scan "src" so that
> > any sub-operands are checked,  the number of REG and non-constant
> > objects are record in "reg_found" and "nonconst_found".  the embedded
> > register found also record in the "reg" field of the structure
> > "rtx_search_arg",
> Constants in this context are going to satisfy CONSTANT_P, you don't
> need to manually verify them.  It will include simple constants and
> constants which are built up out of multiple instructions (symbolic
> constants in particular).

Jeff,

 thanks, I partially understand your meaning here.

  take the function "ira_implicitly_set_insn_hard_regs" in ira-lives.c for example,

  when generating address rtl, gcc will automatically generate "const" operator to prefix
  the address expression, like the following. so a simple CONSTANT_P check is enough in
  case there is no embedded register.

  (insn 309 310 308 3 (set (reg:DI 44 r15 [orig:94 ivtmp.674 ] [94])
        (const:DI (plus:DI (symbol_ref:DI ("recog_data") [flags 0x40]  <var_decl 0x2b2c2ff91510 recog_data>)
                (const_int 480 [0x1e0])))) -1


  but for architecture like aarch64, the following instruction sequences to forming address
  may be generated

(insn 73 14 74 4 (set (reg/f:DI 20 x20 [99])
        (high:DI (symbol_ref:DI ("global_a") [flags 0xc0]  <var_decl 0x7ff755a60900 stats>))) 35 {*movdi_aarch64}
     (expr_list:REG_EQUIV (high:DI (symbol_ref:DI ("global_a") [flags 0xc0]  <var_decl 0x7ff755a60900 stats>))
        (nil)))

(insn 17 30 25 5 (set (reg/f:DI 4 x4 [83])
        (lo_sum:DI (reg/f:DI 20 x20 [99])
            (symbol_ref:DI ("global_a") [flags 0xc0]  <var_decl 0x7ff755a60900 stats>))) {add_losym_di}
     (expr_list:REG_EQUIV (symbol_ref:DI ("global_a") [flags 0xc0]  <var_decl 0x7ff755a60900 stats>)
        (nil)))

 while CONSTANT_P could not catch the latter lo_sum case, as the RTX_CLASS of lo_sum is RTX_OBJ not RTX_CONST_OBJ,
 so the

 the manually verification

      FOR_EACH_SUBRTX_VAR (iter, array, src, ALL)
        {
          rtx x = *iter;
          if (REG_P (x))
            {
              reg_num++;
              src_inner = x;
            }
          else if (!CONSTANT_P (x) && OBJECT_P (x))
            nonconstobj_num++;
        }

 is to catch cases like the lo_sum insn above which contains no more than 1 register and only contains
 RTX_CONST_OBJ.

> I suspect you still need the callback to verify the # of registers is
> just 1 so that the later tests work.  However, please don't use
> for_each_rtx, we're moving away from that to a more efficient walker
> FOR_EACH_SUBRTX.

 
  fixed, patch updated, please review.

  thanks.

  BTW, when I verifying this patch on gilbc x86-64 build, I found there may be another hiding
  issue when calculating live_in when splitting edge in move_insn_for_shrink_wrap, I will reply
  on that historical email thread directly.

Regards,
Jiong

> Jeff
diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
index fd24135..739e957 100644
--- a/gcc/shrink-wrap.c
+++ b/gcc/shrink-wrap.c
@@ -53,6 +53,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "bb-reorder.h"
 #include "shrink-wrap.h"
 #include "regcprop.h"
+#include "rtl-iter.h"
 
 #ifdef HAVE_simple_return
 
@@ -169,7 +170,9 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
 {
   rtx set, src, dest;
   bitmap live_out, live_in, bb_uses, bb_defs;
-  unsigned int i, dregno, end_dregno, sregno, end_sregno;
+  unsigned int i, dregno, end_dregno;
+  unsigned int sregno = FIRST_PSEUDO_REGISTER;
+  unsigned int end_sregno = FIRST_PSEUDO_REGISTER;
   basic_block next_block;
   edge live_edge;
 
@@ -179,7 +182,34 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
     return false;
   src = SET_SRC (set);
   dest = SET_DEST (set);
-  if (!REG_P (dest) || !REG_P (src)
+
+  if (!REG_P (src))
+    {
+      unsigned int reg_num = 0;
+      unsigned int nonconstobj_num = 0;
+      rtx src_inner = NULL_RTX;
+
+      subrtx_var_iterator::array_type array;
+      FOR_EACH_SUBRTX_VAR (iter, array, src, ALL)
+	{
+	  rtx x = *iter;
+	  if (REG_P (x))
+	    {
+	      reg_num++;
+	      src_inner = x;
+	    }
+	  else if (!CONSTANT_P (x) && OBJECT_P (x))
+	    nonconstobj_num++;
+	}
+
+      if (nonconstobj_num > 0
+	  || reg_num > 1)
+	src = NULL_RTX;
+      else if (reg_num == 1)
+	src = src_inner;
+    }
+
+  if (!REG_P (dest) || src == NULL_RTX
       /* STACK or FRAME related adjustment might be part of prologue.
 	 So keep them in the entry block.  */
       || dest == stack_pointer_rtx
@@ -188,10 +218,13 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
     return false;
 
   /* Make sure that the source register isn't defined later in BB.  */
-  sregno = REGNO (src);
-  end_sregno = END_REGNO (src);
-  if (overlaps_hard_reg_set_p (defs, GET_MODE (src), sregno))
-    return false;
+  if (REG_P (src))
+    {
+      sregno = REGNO (src);
+      end_sregno = END_REGNO (src);
+      if (overlaps_hard_reg_set_p (defs, GET_MODE (src), sregno))
+	return false;
+    }
 
   /* Make sure that the destination register isn't referenced later in BB.  */
   dregno = REGNO (dest);