diff mbox

Fix handling of ZERO_EXTRACT lhs with REG_EQUAL note in the combiner (PR target/69442)

Message ID 20160126083929.GR3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Jan. 26, 2016, 8:39 a.m. UTC
Hi!

In the middle of last year, Kugan has defined REG_EQUAL notes even for the
case when SET_DEST of the single set is ZERO_EXTRACT, before that I believe
it has been defined only for REG/SUBREG and STRICT_LOW_PART thereof.
But, like for STRICT_LOW_PART, the REG_EQUAL note describes the whole
contained register rather than just the bits that are being set.
Unfortunately, neither the documentation nor the combiner have been changed
for this, so if say on
(insn 7 4 8 2 (set (reg:SI 117)
        (const_int 65535 [0xffff])) pr69442.c:7 614 {*arm_movsi_vfp}
     (nil))
...
(insn 16 14 17 2 (set (reg:SI 123)
        (const_int 0 [0])) pr69442.c:9 614 {*arm_movsi_vfp}
     (nil))
(insn 17 16 18 2 (set (zero_extract:SI (reg:SI 123)
            (const_int 16 [0x10])
            (const_int 16 [0x10]))
        (reg:SI 117)) pr69442.c:9 83 {insv_t2}
     (expr_list:REG_DEAD (reg:SI 117)
        (expr_list:REG_EQUAL (const_int -65536 [0xffffffffffff0000])
            (nil))))
the combiner would replace the SET_SRC of the set with
-65536, but that has different meaning, it says that the upper 16 bits
of a register containing zero are supposed to be set to -65536 (and as we
care just about 16 bits, that is in fact 0).  So, either we could shift
the constant around and say that the zero extract is set to
-65536 >> 16 in this case, but then we don't say anything about the other
bits and if e.g. there are multiple setters of the earlier 123 pseudo value,
we'd give up, or as the patch does, it says the whole inner register
is set to the given constant.  This setting doesn't have to be recognized as
valid, the whole point of the temporary assignment is just improve the
try_combine and then revert it.

Bootstrapped/regtested on x86_64-linux and i686-linux, Kugan has kindly
tested it on arm too (my armhfp bootstrap/regtest of this is progressing too
slowly).  Ok for trunk?

2016-01-26  Jakub Jelinek  <jakub@redhat.com>

	PR target/69442
	* combine.c (combine_instructions): For REG_EQUAL note with
	SET_DEST being ZERO_EXTRACT, also temporarily set SET_DEST
	to the underlying register.
	* doc/rtl.texi (REG_EQUAL): Document the behavior of
	REG_EQUAL/REG_EQUIV notes if SET_DEST is ZERO_EXTRACT.

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


	Jakub

Comments

Bernd Schmidt Jan. 26, 2016, 10:23 a.m. UTC | #1
On 01/26/2016 09:39 AM, Jakub Jelinek wrote:
>
> 	PR target/69442
> 	* combine.c (combine_instructions): For REG_EQUAL note with
> 	SET_DEST being ZERO_EXTRACT, also temporarily set SET_DEST
> 	to the underlying register.
> 	* doc/rtl.texi (REG_EQUAL): Document the behavior of
> 	REG_EQUAL/REG_EQUIV notes if SET_DEST is ZERO_EXTRACT.
>
> 	* gcc.dg/pr69442.c: New test.

Ok.


Bernd
diff mbox

Patch

--- gcc/combine.c.jj	2016-01-14 16:10:24.000000000 +0100
+++ gcc/combine.c	2016-01-25 21:03:35.572594150 +0100
@@ -1454,15 +1454,21 @@  combine_instructions (rtx_insn *f, unsig
 		  && ! unmentioned_reg_p (note, SET_SRC (set))
 		  && (GET_MODE (note) == VOIDmode
 		      ? SCALAR_INT_MODE_P (GET_MODE (SET_DEST (set)))
-		      : GET_MODE (SET_DEST (set)) == GET_MODE (note)))
+		      : (GET_MODE (SET_DEST (set)) == GET_MODE (note)
+			 && (GET_CODE (SET_DEST (set)) != ZERO_EXTRACT
+			     || (GET_MODE (XEXP (SET_DEST (set), 0))
+				 == GET_MODE (note))))))
 		{
 		  /* Temporarily replace the set's source with the
 		     contents of the REG_EQUAL note.  The insn will
 		     be deleted or recognized by try_combine.  */
-		  rtx orig = SET_SRC (set);
+		  rtx orig_src = SET_SRC (set);
+		  rtx orig_dest = SET_DEST (set);
+		  if (GET_CODE (SET_DEST (set)) == ZERO_EXTRACT)
+		    SET_DEST (set) = XEXP (SET_DEST (set), 0);
 		  SET_SRC (set) = note;
 		  i2mod = temp;
-		  i2mod_old_rhs = copy_rtx (orig);
+		  i2mod_old_rhs = copy_rtx (orig_src);
 		  i2mod_new_rhs = copy_rtx (note);
 		  next = try_combine (insn, i2mod, NULL, NULL,
 				      &new_direct_jump_p,
@@ -1473,7 +1479,8 @@  combine_instructions (rtx_insn *f, unsig
 		      statistics_counter_event (cfun, "insn-with-note combine", 1);
 		      goto retry;
 		    }
-		  SET_SRC (set) = orig;
+		  SET_SRC (set) = orig_src;
+		  SET_DEST (set) = orig_dest;
 		}
 	    }
 
--- gcc/doc/rtl.texi.jj	2016-01-04 14:55:58.000000000 +0100
+++ gcc/doc/rtl.texi	2016-01-25 18:51:23.813258380 +0100
@@ -3915,9 +3915,9 @@  indicates that that register will be equ
 scope of this equivalence differs between the two types of notes.  The
 value which the insn explicitly copies into the register may look
 different from @var{op}, but they will be equal at run time.  If the
-output of the single @code{set} is a @code{strict_low_part} expression,
-the note refers to the register that is contained in @code{SUBREG_REG}
-of the @code{subreg} expression.
+output of the single @code{set} is a @code{strict_low_part} or
+@code{zero_extract} expression, the note refers to the register that
+is contained in its first operand.
 
 For @code{REG_EQUIV}, the register is equivalent to @var{op} throughout
 the entire function, and could validly be replaced in all its
--- gcc/testsuite/gcc.dg/pr69442.c.jj	2016-01-25 20:58:35.684703880 +0100
+++ gcc/testsuite/gcc.dg/pr69442.c	2016-01-25 20:58:14.000000000 +0100
@@ -0,0 +1,23 @@ 
+/* PR target/69442 */
+/* { dg-do run } */
+/* { dg-options "-Og" } */
+
+unsigned long long __attribute__ ((noinline, noclone))
+foo (unsigned int x, unsigned long long y)
+{
+  x |= 0xffff;
+  y -= 0xffULL;
+  y %= 0xffff0000ffffffe7ULL;
+  return x + y;
+}
+
+int
+main ()
+{
+  if (sizeof (unsigned long long) * __CHAR_BIT__ != 64)
+    return 0;
+
+  if (foo (0, 0) != 0xffff0000ff19ULL)
+    __builtin_abort ();
+  return 0;
+}