[PATCH] PR63404, gcc 5 miscompiles linux block layer

[Richard Henderson]

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 <law@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~

-------------- next part --------------
2014-10-10  Richard Henderson  <rth@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))
     {