On 10/08/2014 08:31 AM, Jiong Wang wrote:
> 
> On 30/09/14 19:36, Jiong Wang wrote:
>> 2014-09-30 17:30 GMT+01:00 Jeff Law <l...@redhat.com>:
>>> On 09/30/14 08:37, Jiong Wang wrote:
>>>>
>>>> On 30/09/14 05:21, Jeff Law wrote:
>>>>
>>>>> I do agree with Richard that it would be useful to see the insns that
>>>>> are incorrectly sunk and the surrounding context.
>>> So I must be missing something.  I thought the shrink-wrapping code wouldn't
>>> sink arithmetic/logical insns like we see with insn 14 and insn 182.  I
>>> thought it was limited to reg-reg copies and constant initializations.
>> yes, it was limited to reg-reg copies, and my previous sink improvement 
>> aimed to
>> sink any rtx
>>
>>    A: be single_set
>>    B: the src operand be any combination of no more than one register
>> and no non-constant objects.
>>
>> while some operator like shift may have side effect. IMHO, all side
>> effects are reflected on RTX,
>> together with this fail_on_clobber_use modification, the rtx returned
>> by single_set_no_clobber_use is
>> safe to sink if it meets the above limit B and pass later register
>> use/def check in move_insn_for_shrink_wrap ?
> 
> Ping ~
> 
> And as there is NONDEBUG_INSN_P check before move_insn_for_shrink_wrap 
> invoked,
> we could avoid creating new wrapper function by invoke single_set_2 directly.

I'm committing the following to fix this.

(1) Don't bother modifying single_set; just look for a bare SET.
(2) Tighten the set of expressions we're willing to move.
(3) Use direct "return false" in the failure case, rather than
    counting a non-zero number of non-constants, etc.

Tested on x86_64, and against Andi's test case (unfortunately unreduced).


r~

2014-10-10  Richard Henderson  <r...@redhat.com>

        PR target/63404
        * shrink-wrap.c (move_insn_for_shrink_wrap): Don't use single_set.
        Restrict the set of expressions we're willing to move.


diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
index b1ff8a2..257812c 100644
--- a/gcc/shrink-wrap.c
+++ b/gcc/shrink-wrap.c
@@ -176,17 +176,40 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
   basic_block next_block;
   edge live_edge;
 
-  /* Look for a simple register copy.  */
-  set = single_set (insn);
-  if (!set)
+  /* Look for a simple register assignment.  We don't use single_set here
+     because we can't deal with any CLOBBERs, USEs, or REG_UNUSED secondary
+     destinations.  */
+  if (!INSN_P (insn))
+    return false;
+  set = PATTERN (insn);
+  if (GET_CODE (set) != SET)
     return false;
   src = SET_SRC (set);
   dest = SET_DEST (set);
 
-  if (!REG_P (src))
+  /* For the destination, we want only a register.  Also disallow STACK
+     or FRAME related adjustments.  They are likely part of the prologue,
+     so keep them in the entry block.  */
+  if (!REG_P (dest)
+      || dest == stack_pointer_rtx
+      || dest == frame_pointer_rtx
+      || dest == hard_frame_pointer_rtx)
+    return false;
+
+  /* For the source, we want one of:
+      (1) A (non-overlapping) register
+      (2) A constant,
+      (3) An expression involving no more than one register.
+
+     That last point comes from the code following, which was originally
+     written to handle only register move operations, and still only handles
+     a single source register when checking for overlaps.  Happily, the
+     same checks can be applied to expressions like (plus reg const).  */
+
+  if (CONSTANT_P (src))
+    ;
+  else if (!REG_P (src))
     {
-      unsigned int reg_num = 0;
-      unsigned int nonconstobj_num = 0;
       rtx src_inner = NULL_RTX;
 
       if (can_throw_internal (insn))
@@ -196,30 +219,50 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
       FOR_EACH_SUBRTX_VAR (iter, array, src, ALL)
        {
          rtx x = *iter;
-         if (REG_P (x))
+         switch (GET_RTX_CLASS (GET_CODE (x)))
            {
-             reg_num++;
-             src_inner = x;
+           case RTX_CONST_OBJ:
+           case RTX_COMPARE:
+           case RTX_COMM_COMPARE:
+           case RTX_BIN_ARITH:
+           case RTX_COMM_ARITH:
+           case RTX_UNARY:
+           case RTX_TERNARY:
+             /* Constant or expression.  Continue.  */
+             break;
+
+           case RTX_OBJ:
+           case RTX_EXTRA:
+             switch (GET_CODE (x))
+               {
+               case UNSPEC:
+               case SUBREG:
+               case STRICT_LOW_PART:
+               case PC:
+                 /* Ok.  Continue.  */
+                 break;
+
+               case REG:
+                 /* Fail if we see a second inner register.  */
+                 if (src_inner != NULL)
+                   return false;
+                 src_inner = x;
+                 break;
+
+               default:
+                 return false;
+               }
+             break;
+
+           default:
+             return false;
            }
-         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)
+      if (src_inner != NULL)
        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
-      || dest == frame_pointer_rtx
-      || dest == hard_frame_pointer_rtx)
-    return false;
-
   /* Make sure that the source register isn't defined later in BB.  */
   if (REG_P (src))
     {

Reply via email to