diff mbox

Adjust/remove REG_EQ{UAL,UIV} notes in REE (PR rtl-optimization/63659)

Message ID 20141027204720.GZ10376@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Oct. 27, 2014, 8:47 p.m. UTC 
Hi!

The following testcase is miscompiled in 4.8+ on x86_64 at -O2+,
because REE widens for ZERO_EXTEND mode on
(set (reg:QI ax) (const_int -1)) instruction to SImode, but doesn't
adjust REG_EQUAL note of (const_int -1) also to (const_int 0xff)
like it changes the pattern.

The following patch attempts to adjust the notes if they are CONST_INT,
or drop otherwise (not sure if it would be desirable to emit
a ZERO_EXTEND of the previous content of the note), and both through
validate_change, so that if we give up later on, it is undone.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
What about 4.9.2 (after release) and 4.8.x?

2014-10-27  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/63659
	* ree.c (update_reg_equal_equiv_notes): New function.
	(combine_set_extension, transform_ifelse): Use it.

	* gcc.c-torture/execute/pr63659.c: New test.


	Jakub

Comments

Jeff Law Oct. 30, 2014, 3:47 a.m. UTC | #1 
On 10/27/14 14:47, Jakub Jelinek wrote:
> Hi!
>
> The following testcase is miscompiled in 4.8+ on x86_64 at -O2+,
> because REE widens for ZERO_EXTEND mode on
> (set (reg:QI ax) (const_int -1)) instruction to SImode, but doesn't
> adjust REG_EQUAL note of (const_int -1) also to (const_int 0xff)
> like it changes the pattern.
>
> The following patch attempts to adjust the notes if they are CONST_INT,
> or drop otherwise (not sure if it would be desirable to emit
> a ZERO_EXTEND of the previous content of the note), and both through
> validate_change, so that if we give up later on, it is undone.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> What about 4.9.2 (after release) and 4.8.x?
>
> 2014-10-27  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR rtl-optimization/63659
> 	* ree.c (update_reg_equal_equiv_notes): New function.
> 	(combine_set_extension, transform_ifelse): Use it.
>
> 	* gcc.c-torture/execute/pr63659.c: New test.
Does this do the right thing in the case where there's multiple reaching 
definitions and one of those reaching definitions turns out to be 
non-combinable, while some other has a fixable REG_EQUAL note?

Don't we have to separate the check for a fixable note from the actual 
changing of the note?

Jeff
Jakub Jelinek Oct. 30, 2014, 7:36 a.m. UTC | #2 
On Wed, Oct 29, 2014 at 09:47:33PM -0600, Jeff Law wrote:
> On 10/27/14 14:47, Jakub Jelinek wrote:
> >Hi!
> >
> >The following testcase is miscompiled in 4.8+ on x86_64 at -O2+,
> >because REE widens for ZERO_EXTEND mode on
> >(set (reg:QI ax) (const_int -1)) instruction to SImode, but doesn't
> >adjust REG_EQUAL note of (const_int -1) also to (const_int 0xff)
> >like it changes the pattern.
> >
> >The following patch attempts to adjust the notes if they are CONST_INT,
> >or drop otherwise (not sure if it would be desirable to emit
> >a ZERO_EXTEND of the previous content of the note), and both through
> >validate_change, so that if we give up later on, it is undone.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >What about 4.9.2 (after release) and 4.8.x?
> >
> >2014-10-27  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR rtl-optimization/63659
> >	* ree.c (update_reg_equal_equiv_notes): New function.
> >	(combine_set_extension, transform_ifelse): Use it.
> >
> >	* gcc.c-torture/execute/pr63659.c: New test.
> Does this do the right thing in the case where there's multiple reaching
> definitions and one of those reaching definitions turns out to be
> non-combinable, while some other has a fixable REG_EQUAL note?

The changes to REG_EQUAL or REG_EQUIV notes (either adjustment of CONST_INT
in it, or its removal) is tied to the changing of the insn holding it
in the patch.  So, if we revert the changes to the instruction, so it is
again the original mode, we revert the changes to the notes too (either put
back the old CONST_INT, or re-add the note).
The granularity of changes in REE is combine_reaching_defs - each cand
separately, if any insn needed to be changed for some cand fails to be
changed, we revert all the changes for the cand in question (it is not
possible to have only a subset of setters adjusted if we want to adjust
the user(s)).  So IMHO the right thing is what the patch does, tie it to
the apply_change_group/cancel_changes that verifies or undoes the insn
changes.

> Don't we have to separate the check for a fixable note from the actual
> changing of the note?

	Jakub
Jeff Law Oct. 31, 2014, 7:22 p.m. UTC | #3 
On 10/30/14 01:36, Jakub Jelinek wrote:
> On Wed, Oct 29, 2014 at 09:47:33PM -0600, Jeff Law wrote:
>> On 10/27/14 14:47, Jakub Jelinek wrote:
>>> Hi!
>>>
>>> The following testcase is miscompiled in 4.8+ on x86_64 at -O2+,
>>> because REE widens for ZERO_EXTEND mode on
>>> (set (reg:QI ax) (const_int -1)) instruction to SImode, but doesn't
>>> adjust REG_EQUAL note of (const_int -1) also to (const_int 0xff)
>>> like it changes the pattern.
>>>
>>> The following patch attempts to adjust the notes if they are CONST_INT,
>>> or drop otherwise (not sure if it would be desirable to emit
>>> a ZERO_EXTEND of the previous content of the note), and both through
>>> validate_change, so that if we give up later on, it is undone.
>>>
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>> What about 4.9.2 (after release) and 4.8.x?
>>>
>>> 2014-10-27  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR rtl-optimization/63659
>>> 	* ree.c (update_reg_equal_equiv_notes): New function.
>>> 	(combine_set_extension, transform_ifelse): Use it.
>>>
>>> 	* gcc.c-torture/execute/pr63659.c: New test.
>> Does this do the right thing in the case where there's multiple reaching
>> definitions and one of those reaching definitions turns out to be
>> non-combinable, while some other has a fixable REG_EQUAL note?
>
> The changes to REG_EQUAL or REG_EQUIV notes (either adjustment of CONST_INT
> in it, or its removal) is tied to the changing of the insn holding it
> in the patch.

Doh!  I should have realized that.  Objection removed.  OK for the trunk.

jeff
diff mbox

Patch

--- gcc/ree.c.jj	2014-10-22 15:52:18.000000000 +0200
+++ gcc/ree.c	2014-10-27 19:18:37.287412478 +0100
@@ -266,6 +266,50 @@  typedef struct ext_cand
 
 static int max_insn_uid;
 
+/* Update or remove REG_EQUAL or REG_EQUIV notes for INSN.  */
+
+static bool
+update_reg_equal_equiv_notes (rtx_insn *insn, enum machine_mode new_mode,
+			      enum machine_mode old_mode, enum rtx_code code)
+{
+  rtx *loc = &REG_NOTES (insn);
+  while (*loc)
+    {
+      enum reg_note kind = REG_NOTE_KIND (*loc);
+      if (kind == REG_EQUAL || kind == REG_EQUIV)
+	{
+	  rtx orig_src = XEXP (*loc, 0);
+	  /* Update equivalency constants.  Recall that RTL constants are
+	     sign-extended.  */
+	  if (GET_CODE (orig_src) == CONST_INT
+	      && HOST_BITS_PER_WIDE_INT >= GET_MODE_BITSIZE (new_mode))
+	    {
+	      if (INTVAL (orig_src) >= 0 || code == SIGN_EXTEND)
+		/* Nothing needed.  */;
+	      else
+		{
+		  /* Zero-extend the negative constant by masking out the
+		     bits outside the source mode.  */
+		  rtx new_const_int
+		    = gen_int_mode (INTVAL (orig_src)
+				    & GET_MODE_MASK (old_mode),
+				    new_mode);
+		  if (!validate_change (insn, &XEXP (*loc, 0),
+					new_const_int, true))
+		    return false;
+		}
+	      loc = &XEXP (*loc, 1);
+	    }
+	  /* Drop all other notes, they assume a wrong mode.  */
+	  else if (!validate_change (insn, loc, XEXP (*loc, 1), true))
+	    return false;
+	}
+      else
+	loc = &XEXP (*loc, 1);
+    }
+  return true;
+}
+
 /* Given a insn (CURR_INSN), an extension candidate for removal (CAND)
    and a pointer to the SET rtx (ORIG_SET) that needs to be modified,
    this code modifies the SET rtx to a new SET rtx that extends the
@@ -287,6 +331,7 @@  static bool
 combine_set_extension (ext_cand *cand, rtx_insn *curr_insn, rtx *orig_set)
 {
   rtx orig_src = SET_SRC (*orig_set);
+  enum machine_mode orig_mode = GET_MODE (SET_DEST (*orig_set));
   rtx new_set;
   rtx cand_pat = PATTERN (cand->insn);
 
@@ -323,9 +368,8 @@  combine_set_extension (ext_cand *cand, r
 	{
 	  /* Zero-extend the negative constant by masking out the bits outside
 	     the source mode.  */
-	  enum machine_mode src_mode = GET_MODE (SET_DEST (*orig_set));
 	  rtx new_const_int
-	    = gen_int_mode (INTVAL (orig_src) & GET_MODE_MASK (src_mode),
+	    = gen_int_mode (INTVAL (orig_src) & GET_MODE_MASK (orig_mode),
 			    GET_MODE (new_reg));
 	  new_set = gen_rtx_SET (VOIDmode, new_reg, new_const_int);
 	}
@@ -364,7 +408,9 @@  combine_set_extension (ext_cand *cand, r
 
   /* This change is a part of a group of changes.  Hence,
      validate_change will not try to commit the change.  */
-  if (validate_change (curr_insn, orig_set, new_set, true))
+  if (validate_change (curr_insn, orig_set, new_set, true)
+      && update_reg_equal_equiv_notes (curr_insn, cand->mode, orig_mode,
+				       cand->code))
     {
       if (dump_file)
         {
@@ -414,7 +460,9 @@  transform_ifelse (ext_cand *cand, rtx_in
   ifexpr = gen_rtx_IF_THEN_ELSE (cand->mode, cond, map_srcreg, map_srcreg2);
   new_set = gen_rtx_SET (VOIDmode, map_dstreg, ifexpr);
 
-  if (validate_change (def_insn, &PATTERN (def_insn), new_set, true))
+  if (validate_change (def_insn, &PATTERN (def_insn), new_set, true)
+      && update_reg_equal_equiv_notes (def_insn, cand->mode, GET_MODE (dstreg),
+				       cand->code))
     {
       if (dump_file)
         {
--- gcc/testsuite/gcc.c-torture/execute/pr63659.c.jj	2014-10-27 19:26:57.720902738 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr63659.c	2014-10-27 19:26:36.000000000 +0100
@@ -0,0 +1,29 @@ 
+/* PR rtl-optimization/63659 */
+
+int a, b, c, *d = &b, g, h, i;
+unsigned char e;
+char f;
+
+int
+main ()
+{
+  while (a)
+    {
+      for (a = 0; a; a++)
+	for (; c; c++)
+	  ;
+      if (i)
+	break;
+    }
+
+  char j = c, k = -1, l;
+  l = g = j >> h;
+  f = l == 0 ? k : k % l;
+  e = 0 ? 0 : f;
+  *d = e;
+
+  if (b != 255)
+    __builtin_abort ();
+
+  return 0;
+}