diff mbox series

combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830]

Message ID 20210409074930.GB1179226@tucnak
State New
Headers show
Series combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] | expand

Commit Message

Jakub Jelinek April 9, 2021, 7:49 a.m. UTC 
Hi!

On the following testcase on aarch64 the combiner propagates
(clobber:TI (const_int 0)) into a DEBUG_INSN.  Such clobbers are
specific to the combiner, created by gen_lowpart_for_combine:
 fail:
  return gen_rtx_CLOBBER (omode, const0_rtx);
which can be embedded anywhere and the combiner hopes they never match
anything.  That is hopefully the case of all instructions that go through
recog, but for DEBUG_INSNs we don't have any strict rules on what matches
and what doesn't and so if combiner calls propagate_for_debug with
src containing such clobbers embedded in it, it will happily propagate it
into DEBUG_INSNs and cause problems later (in this case ICEs during LRA).

The following patch ensures that if such clobbers would be propagated into
some DEBUG_INSNs that such DEBUG_INSNs are reset to the unknown state.
propagate_for_debug uses volatile_insn_p check to determine if something
needs to be reset after propagation and the patch replaces the combiner
specific clobbers with UNSPEC_VOLATILE that volatile_insn_p will return
true on and thus valtrack.c reset those DEBUG_INSNs.

Bootstrapped/regtested on x86_64-linux and i686-linux and tested in
cross to aarch64-linux on the testcase, ok for trunk?

2021-04-09  Jakub Jelinek  <jakub@redhat.com>

	PR debug/99830
	* combine.c (combine_propagate_for_debug): New function.
	(try_combine): Use combine_propagate_for_debug instead of
	propagate_for_debug.

	* gcc.dg/pr99830.c: New test.


	Jakub

Comments

Segher Boessenkool April 12, 2021, 10:49 p.m. UTC | #1 
Hi!

On Fri, Apr 09, 2021 at 09:49:30AM +0200, Jakub Jelinek wrote:
> On the following testcase on aarch64 the combiner propagates
> (clobber:TI (const_int 0)) into a DEBUG_INSN.  Such clobbers are
> specific to the combiner, created by gen_lowpart_for_combine:
>  fail:
>   return gen_rtx_CLOBBER (omode, const0_rtx);
> which can be embedded anywhere and the combiner hopes they never match
> anything.

It doesn't just hope that, there is code all over the place that
guarantees this is true.  Yes, that isn't super robust, I agree.  But
that is what we have until someone manages to change it.

> That is hopefully the case of all instructions that go through
> recog, but for DEBUG_INSNs we don't have any strict rules on what matches
> and what doesn't and so if combiner calls propagate_for_debug with
> src containing such clobbers embedded in it, it will happily propagate it
> into DEBUG_INSNs and cause problems later (in this case ICEs during LRA).

It should never get that far.

(We talked about this; just adding it here to archive it for posterity.)


Segher
diff mbox series

Patch

--- gcc/combine.c.jj	2021-04-07 12:35:03.060224972 +0200
+++ gcc/combine.c	2021-04-08 16:22:17.851459742 +0200
@@ -2610,6 +2610,30 @@  count_auto_inc (rtx, rtx, rtx, rtx, rtx,
   return 0;
 }
 
+/* Wrapper around propagate_for_debug to deal with gen_lowpart_for_combine
+   (clobber (const_int 0)).  In normal insns a clobber of const0_rtx nested
+   somewhere will cause the pattern not to be recognized, but in debug insns
+   it doesn't, one needs to use something on which volatile_insn_p is true
+   instead.  */
+
+static void
+combine_propagate_for_debug (rtx_insn *insn, rtx_insn *last, rtx dest, rtx src,
+			     basic_block this_basic_block)
+{
+  subrtx_iterator::array_type array;
+  FOR_EACH_SUBRTX (iter, array, src, ALL)
+    {
+      const_rtx x = *iter;
+      if (GET_CODE (x) == CLOBBER && XEXP (x, 0) == const0_rtx)
+	{
+	  src = gen_rtx_UNSPEC_VOLATILE (GET_MODE (dest),
+					 gen_rtvec (1, const0_rtx), -1);
+	  break;
+	}
+    }
+  propagate_for_debug (insn, last, dest, src, this_basic_block);
+}
+
 /* Try to combine the insns I0, I1 and I2 into I3.
    Here I0, I1 and I2 appear earlier than I3.
    I0 and I1 can be zero; then we combine just I2 into I3, or I1 and I2 into
@@ -4200,8 +4224,8 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 		   i2src while its original mode is temporarily
 		   restored, and then clear i2scratch so that we don't
 		   do it again later.  */
-		propagate_for_debug (i2, last_combined_insn, reg, i2src,
-				     this_basic_block);
+		combine_propagate_for_debug (i2, last_combined_insn, reg,
+					     i2src, this_basic_block);
 		i2scratch = false;
 		/* Put back the new mode.  */
 		adjust_reg_mode (reg, new_mode);
@@ -4235,12 +4259,13 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 		   with this copy we have created; then, replace the
 		   copy with the SUBREG of the original shared reg,
 		   once again changed to the new mode.  */
-		propagate_for_debug (first, last, reg, tempreg,
-				     this_basic_block);
+		combine_propagate_for_debug (first, last, reg, tempreg,
+					     this_basic_block);
 		adjust_reg_mode (reg, new_mode);
-		propagate_for_debug (first, last, tempreg,
-				     lowpart_subreg (old_mode, reg, new_mode),
-				     this_basic_block);
+		combine_propagate_for_debug (first, last, tempreg,
+					     lowpart_subreg (old_mode, reg,
+							     new_mode),
+					     this_basic_block);
 	      }
 	  }
     }
@@ -4499,16 +4524,16 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
     if (newi2pat)
       {
 	if (MAY_HAVE_DEBUG_BIND_INSNS && i2scratch)
-	  propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
-			       this_basic_block);
+	  combine_propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
+				       this_basic_block);
 	INSN_CODE (i2) = i2_code_number;
 	PATTERN (i2) = newi2pat;
       }
     else
       {
 	if (MAY_HAVE_DEBUG_BIND_INSNS && i2src)
-	  propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
-			       this_basic_block);
+	  combine_propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
+				       this_basic_block);
 	SET_INSN_DELETED (i2);
       }
 
@@ -4517,8 +4542,8 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 	LOG_LINKS (i1) = NULL;
 	REG_NOTES (i1) = 0;
 	if (MAY_HAVE_DEBUG_BIND_INSNS)
-	  propagate_for_debug (i1, last_combined_insn, i1dest, i1src,
-			       this_basic_block);
+	  combine_propagate_for_debug (i1, last_combined_insn, i1dest, i1src,
+				       this_basic_block);
 	SET_INSN_DELETED (i1);
       }
 
@@ -4527,8 +4552,8 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 	LOG_LINKS (i0) = NULL;
 	REG_NOTES (i0) = 0;
 	if (MAY_HAVE_DEBUG_BIND_INSNS)
-	  propagate_for_debug (i0, last_combined_insn, i0dest, i0src,
-			       this_basic_block);
+	  combine_propagate_for_debug (i0, last_combined_insn, i0dest, i0src,
+				       this_basic_block);
 	SET_INSN_DELETED (i0);
       }
 
--- gcc/testsuite/gcc.dg/pr99830.c.jj	2021-04-08 16:39:44.243774333 +0200
+++ gcc/testsuite/gcc.dg/pr99830.c	2021-04-08 16:28:40.463175738 +0200
@@ -0,0 +1,10 @@ 
+/* PR debug/99830 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -fno-expensive-optimizations -fno-split-wide-types -g" } */
+
+int foo (long a, __int128 b, short c, int d, unsigned e, __int128 f)
+{
+  __builtin_memmove (2 + (char *) &f, foo, 1);
+  c >>= (char) f;
+  return c;
+}