diff mbox

Fix up make_compound_operation (PR rtl-optimization/57130)

Message ID 20130502173556.GX28963@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek May 2, 2013, 5:35 p.m. UTC 
Hi!

As described in the PR, in some cases it is unsafe for
make_compound_operation, if called with in_code == COMPARE,
to pass through that value to make_compound_operation on
the SUBREG_REG of a SUBREG.

My understanding is that in_code == COMPARE (as opposed to
in_code == SET) is mostly harmless, just tells make_extraction
to no longer special case zero extraction at position 0, but there is one
exception - AND with constant power of two CONST_INT.
If we have
make_compound_operation ( (subreg:SI (and:DI (reg:DI) (const_int 0x800000000)) 0), COMPARE)
then
make_compound_operation ( (and:DI (reg:DI) (const_int 0x800000000)), COMPARE)
returns extraction of the 35th bit, and subreg of that is again either zero
or one, but the original subreg is always 0.

Fixed by passing through SET instead of in_code to the recursive invocation,
if
1) the subreg isn't lowpart
2) nested SUBREGs (should be usually simplified, but just in case it hasn't been yet)
3) if subreg's operand is AND with power of two CONST_INT above the bitsize
   of the outer mode

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?

2013-05-02  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/57130
	* combine.c (make_compound_operation): For SUBREG, pass
	SET instead of COMPARE as in_code to the recursive call
	if needed.

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


	Jakub

Comments

Eric Botcazou May 3, 2013, 8:13 a.m. UTC | #1 
> My understanding is that in_code == COMPARE (as opposed to
> in_code == SET) is mostly harmless, just tells make_extraction
> to no longer special case zero extraction at position 0, but there is one
> exception - AND with constant power of two CONST_INT.
> If we have
> make_compound_operation ( (subreg:SI (and:DI (reg:DI) (const_int
> 0x800000000)) 0), COMPARE) then
> make_compound_operation ( (and:DI (reg:DI) (const_int 0x800000000)),
> COMPARE) returns extraction of the 35th bit, and subreg of that is again
> either zero or one, but the original subreg is always 0.
> 
> Fixed by passing through SET instead of in_code to the recursive
> invocation, if
> 1) the subreg isn't lowpart
> 2) nested SUBREGs (should be usually simplified, but just in case it hasn't
> been yet) 3) if subreg's operand is AND with power of two CONST_INT above
> the bitsize of the outer mode

I don't see the need for the 2): wouldn't potentially problematic cases be 
handled by the recursive call to make_compound_operation?  lowpart SUBREGs 
cannot mask out bit #0, can they?  Otherwise this is OK modulo...

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
> 
> 2013-05-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/57130
> 	* combine.c (make_compound_operation): For SUBREG, pass
> 	SET instead of COMPARE as in_code to the recursive call
> 	if needed.

	* combine.c (make_compound_operation) <SUBREG>: Pass...
Jakub Jelinek May 3, 2013, 9:22 a.m. UTC | #2 
On Fri, May 03, 2013 at 10:13:17AM +0200, Eric Botcazou wrote:
> > My understanding is that in_code == COMPARE (as opposed to
> > in_code == SET) is mostly harmless, just tells make_extraction
> > to no longer special case zero extraction at position 0, but there is one
> > exception - AND with constant power of two CONST_INT.
> > If we have
> > make_compound_operation ( (subreg:SI (and:DI (reg:DI) (const_int
> > 0x800000000)) 0), COMPARE) then
> > make_compound_operation ( (and:DI (reg:DI) (const_int 0x800000000)),
> > COMPARE) returns extraction of the 35th bit, and subreg of that is again
> > either zero or one, but the original subreg is always 0.
> > 
> > Fixed by passing through SET instead of in_code to the recursive
> > invocation, if
> > 1) the subreg isn't lowpart
> > 2) nested SUBREGs (should be usually simplified, but just in case it hasn't
> > been yet) 3) if subreg's operand is AND with power of two CONST_INT above
> > the bitsize of the outer mode
> 
> I don't see the need for the 2): wouldn't potentially problematic cases be 
> handled by the recursive call to make_compound_operation?  lowpart SUBREGs 
> cannot mask out bit #0, can they?  Otherwise this is OK modulo...

Consider for whatever reason still unsimplified:
(ne (subreg:QI (subreg:SI (and:DI (reg:DI) (const_int 0x08000000)) 0) 0) (const_int 0))
We'll call
make_compound_operation ( subreg:QI , COMPARE )
here and if 2) isn't there, we'll call
make_compound_operation ( subreg:SI , COMPARE )
and as 0x08000000 is a SImode power-of-two, we call
make_compound_operation ( and:DI , COMPARE )
and thus return (lshiftrt:DI (reg:DI) (const_int 27)), afterwards subregged
to SI mode (still fine), but then to QI mode (wrong).
If there is just simplified
(ne (subreg:QI (and:DI (reg:DI) (const_int 0x08000000)) 0) (const_int 0))
we'd call
make_compound_operation ( subreg:QI , COMPARE )
and, as 0x08000000 is not a QImode power-of-two, we'd just
make_compound_operation ( and:DI , SET )
and all would be fine.

Does it hurt to punt on nested SUBREG (it isn't really punting, just passing
SET instead of COMPARE, which means avoiding the and with power of two and
some make_extraction details), when usually the nested subregs should be
already simplified and thus the reason I want it there is just to be safe?

> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
> > 
> > 2013-05-02  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR rtl-optimization/57130
> > 	* combine.c (make_compound_operation): For SUBREG, pass
> > 	SET instead of COMPARE as in_code to the recursive call
> > 	if needed.
> 
> 	* combine.c (make_compound_operation) <SUBREG>: Pass...

Ok, will tweak that.

	Jakub
Eric Botcazou May 3, 2013, 9:43 a.m. UTC | #3 
> Does it hurt to punt on nested SUBREG (it isn't really punting, just passing
> SET instead of COMPARE, which means avoiding the and with power of two and
> some make_extraction details), when usually the nested subregs should be
> already simplified and thus the reason I want it there is just to be safe?

No, I was reasoning backward, the patch is OK as-is, thanks.
diff mbox

Patch

--- gcc/combine.c.jj	2013-04-11 09:09:39.000000000 +0200
+++ gcc/combine.c	2013-05-02 12:46:07.540196281 +0200
@@ -7697,8 +7697,24 @@  make_compound_operation (rtx x, enum rtx
 	 what it originally did, do this SUBREG as a force_to_mode.  */
       {
 	rtx inner = SUBREG_REG (x), simplified;
-	
-	tem = make_compound_operation (inner, in_code);
+	enum rtx_code subreg_code = in_code;
+
+	/* If in_code is COMPARE, it isn't always safe to pass it through
+	   to the recursive make_compound_operation call.  */
+	if (subreg_code == COMPARE
+	    && (!subreg_lowpart_p (x)
+		|| GET_CODE (inner) == SUBREG
+		/* (subreg:SI (and:DI (reg:DI) (const_int 0x800000000)) 0)
+		   is (const_int 0), rather than
+		   (subreg:SI (lshiftrt:DI (reg:DI) (const_int 35)) 0).  */
+		|| (GET_CODE (inner) == AND
+		    && CONST_INT_P (XEXP (inner, 1))
+		    && GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (inner))
+		    && exact_log2 (UINTVAL (XEXP (inner, 1)))
+		       >= GET_MODE_BITSIZE (mode))))
+	  subreg_code = SET;
+
+	tem = make_compound_operation (inner, subreg_code);
 
 	simplified
 	  = simplify_subreg (mode, tem, GET_MODE (inner), SUBREG_BYTE (x));
--- gcc/testsuite/gcc.c-torture/execute/pr57130.c.jj	2013-05-02 10:52:00.389263977 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr57130.c	2013-05-02 10:51:45.000000000 +0200
@@ -0,0 +1,21 @@ 
+/* PR rtl-optimization/57130 */
+
+struct S { int a, b, c, d; } s[2] = { { 6, 8, -8, -5 }, { 0, 2, -1, 2 } };
+
+__attribute__((noinline, noclone)) void
+foo (struct S r)
+{
+  static int cnt;
+  if (__builtin_memcmp (&r, &s[cnt++], sizeof r) != 0)
+    __builtin_abort ();
+}
+
+int
+main ()
+{
+  struct S r = { 6, 8, -8, -5 };
+  foo (r);
+  r = (struct S) { 0, 2, -1, 2 };
+  foo (r);
+  return 0;
+}