diff mbox

Fix PR rtl-optimization/56178

Message ID 2416005.sBZil2GkVQ@polaris
State New
Headers show

Commit Message

Eric Botcazou Feb. 6, 2013, 6:11 p.m. UTC
This is the miscompilation of the Ada front-end (ureal.adb) on x86-64/Linux 
with profiled bootstrap and another instance of the infamous webizer bug, 
whereby a REG_EQUAL note for a dead pseudo-register ends up creating an insn 
that wrongly zeroes another pseudo-register.

The origin is fwprop1: from

(insn 399 205 400 13 (set (reg:DI 282 [ ln ])
        (subreg:DI (reg:TI 189) 0)) /home/eric/svn/gcc/gcc/ada/urealp.adb:622 
87 {*movdi_internal_rex64}
     (nil))

(insn 400 399 208 13 (set (reg:DI 283 [ ln+8 ])
        (subreg:DI (reg:TI 189) 8)) /home/eric/svn/gcc/gcc/ada/urealp.adb:622 
87 {*movdi_internal_rex64}
     (expr_list:REG_DEAD (reg:TI 189)

the pass tries to forward-propagate (subreg:DI (reg:TI 189) 0)) into a bunch 
of insns, which almost always fails and thus generates the much dreaded 
REG_EQUAL notes:

In insn 208, replacing
 (subreg:SI (reg:DI 282 [ ln ]) 0)
 with (subreg:SI (reg:TI 189) 0)
Changes to insn 208 not profitable
 Setting REG_EQUAL note

In insn 210, replacing
 (lshiftrt:DI (reg:DI 282 [ ln ])
        (const_int 32 [0x20]))
 with (lshiftrt:DI (subreg:DI (reg:TI 189) 0)
        (const_int 32 [0x20]))
Changes to insn 210 not profitable
 Setting REG_EQUAL note

In insn 252, replacing
 (lshiftrt:DI (reg:DI 283 [ ln+8 ])
        (const_int 32 [0x20]))
 with (lshiftrt:DI (subreg:DI (reg:TI 189) 8)
        (const_int 32 [0x20]))
Changes to insn 252 not profitable
 Setting REG_EQUAL note

In insn 257, replacing
 (subreg:SI (reg:DI 282 [ ln ]) 0)
 with (subreg:SI (reg:TI 189) 0)
Changes to insn 257 not profitable
 Setting REG_EQUAL note

I think there is no point in creating a REG_EQUAL note when you're trying to 
propagate a pseudo (or a subreg of a pseudo here).  As a matter of fact, 
that's what CSE explicitly does not:

      /* If this is a single SET, we are setting a register, and we have an
	 equivalent constant, we want to add a REG_NOTE.   We don't want
	 to write a REG_EQUAL note for a constant pseudo since verifying that
	 that pseudo hasn't been eliminated is a pain.  Such a note also
	 won't help anything.

	 Avoid a REG_EQUAL note for (CONST (MINUS (LABEL_REF) (LABEL_REF)))
	 which can be created for a reference to a compile time computable
	 entry in a jump table.  */

      if (n_sets == 1 && src_const && REG_P (dest)
	  && !REG_P (src_const)

so we should not do it in fwprop either.  Patch attached (which adds the 
SUBREG case to cse.c as well); it introduces no changes at -O2 for the 
testcases in gcc.c-torture/compile.  Tested on x86-64/Linux.

Thoughts?


2013-02-06  Eric Botcazou  <ebotcazou@adacore.com>

	PR rtl-optimization/56178
	* cse.c (cse_insn): Do not create a REG_EQUAL note if the source is a
	SUBREG of a register.  Tidy up related block of code.
	* fwprop.c (orward_propagate_and_simplify): Do not create a REG_EQUAL
	note if the source is a register or a SUBREG of a register.

Comments

Jeff Law Feb. 6, 2013, 7:27 p.m. UTC | #1
On 02/06/2013 11:11 AM, Eric Botcazou wrote:

>
> I think there is no point in creating a REG_EQUAL note when you're trying to
> propagate a pseudo (or a subreg of a pseudo here).  As a matter of fact,
> that's what CSE explicitly does not:
Agreed.  I have a hard time seeing where having a REG_EQUAL note that is 
just a REG or SUBREG (REG) is actually useful -- const/copy propagation 
ought to be able to find those equivalences without the aid of the note. 
  And as we've repeatedly seen, they cause problems.

Beyond 4.8, I'd really like to see this formalized a bit with a checker 
to ensure such notes aren't created and docs for REG_EQUAL notes 
indicating such notes are invalid.

>
>
> 2013-02-06  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	PR rtl-optimization/56178
> 	* cse.c (cse_insn): Do not create a REG_EQUAL note if the source is a
> 	SUBREG of a register.  Tidy up related block of code.
> 	* fwprop.c (orward_propagate_and_simplify): Do not create a REG_EQUAL
> 	note if the source is a register or a SUBREG of a register.
Cleanups to cse look fine, as do the real changes of avoiding creating 
notes for the SUBREG (REG) case for both cse and fwprop.  Please install.


jeff
diff mbox

Patch

Index: fwprop.c
===================================================================
--- fwprop.c	(revision 195803)
+++ fwprop.c	(working copy)
@@ -1316,10 +1316,16 @@  forward_propagate_and_simplify (df_ref u
 	 separately try plugging the definition in the note and simplifying.
 	 And only install a REQ_EQUAL note when the destination is a REG
 	 that isn't mentioned in USE_SET, as the note would be invalid
-	 otherwise.  */
-      set_reg_equal = (note == NULL_RTX && REG_P (SET_DEST (use_set))
-		       && ! reg_mentioned_p (SET_DEST (use_set),
-					     SET_SRC (use_set)));
+	 otherwise.  We also don't want to install a note if we are merely
+	 propagating a pseudo since verifying that this pseudo isn't dead
+	 is a pain; moreover such a note won't help anything.  */
+      set_reg_equal = (note == NULL_RTX
+		       && REG_P (SET_DEST (use_set))
+		       && !REG_P (src)
+		       && !(GET_CODE (src) == SUBREG
+			    && REG_P (SUBREG_REG (src)))
+		       && !reg_mentioned_p (SET_DEST (use_set),
+					    SET_SRC (use_set)));
     }
 
   if (GET_MODE (*loc) == VOIDmode)
Index: cse.c
===================================================================
--- cse.c	(revision 195803)
+++ cse.c	(working copy)
@@ -5311,33 +5311,33 @@  cse_insn (rtx insn)
 	}
 
       /* If this is a single SET, we are setting a register, and we have an
-	 equivalent constant, we want to add a REG_NOTE.   We don't want
-	 to write a REG_EQUAL note for a constant pseudo since verifying that
-	 that pseudo hasn't been eliminated is a pain.  Such a note also
-	 won't help anything.
+	 equivalent constant, we want to add a REG_EQUAL note if the constant
+	 is different from the source.  We don't want to do it for a constant
+	 pseudo since verifying that this pseudo hasn't been eliminated is a
+	 pain; moreover such a note won't help anything.
 
 	 Avoid a REG_EQUAL note for (CONST (MINUS (LABEL_REF) (LABEL_REF)))
 	 which can be created for a reference to a compile time computable
 	 entry in a jump table.  */
-
-      if (n_sets == 1 && src_const && REG_P (dest)
+      if (n_sets == 1
+	  && REG_P (dest)
+	  && src_const
 	  && !REG_P (src_const)
-	  && ! (GET_CODE (src_const) == CONST
-		&& GET_CODE (XEXP (src_const, 0)) == MINUS
-		&& GET_CODE (XEXP (XEXP (src_const, 0), 0)) == LABEL_REF
-		&& GET_CODE (XEXP (XEXP (src_const, 0), 1)) == LABEL_REF))
+	  && !(GET_CODE (src_const) == SUBREG
+	       && REG_P (SUBREG_REG (src_const)))
+	  && !(GET_CODE (src_const) == CONST
+	       && GET_CODE (XEXP (src_const, 0)) == MINUS
+	       && GET_CODE (XEXP (XEXP (src_const, 0), 0)) == LABEL_REF
+	       && GET_CODE (XEXP (XEXP (src_const, 0), 1)) == LABEL_REF)
+	  && !rtx_equal_p (src, src_const))
 	{
-	  /* We only want a REG_EQUAL note if src_const != src.  */
-	  if (! rtx_equal_p (src, src_const))
-	    {
-	      /* Make sure that the rtx is not shared.  */
-	      src_const = copy_rtx (src_const);
+	  /* Make sure that the rtx is not shared.  */
+	  src_const = copy_rtx (src_const);
 
-	      /* Record the actual constant value in a REG_EQUAL note,
-		 making a new one if one does not already exist.  */
-	      set_unique_reg_note (insn, REG_EQUAL, src_const);
-	      df_notes_rescan (insn);
-	    }
+	  /* Record the actual constant value in a REG_EQUAL note,
+	     making a new one if one does not already exist.  */
+	  set_unique_reg_note (insn, REG_EQUAL, src_const);
+	  df_notes_rescan (insn);
 	}
 
       /* Now deal with the destination.  */