Patchwork Fix force_to_mode not to modify in place the passed rtl (PR rtl-optimization/58726)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 3, 2013, 10:46 p.m.
Message ID <20131203224654.GH892@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/296346/
State New
Headers show

Comments

Jakub Jelinek - Dec. 3, 2013, 10:46 p.m.
Hi!

As described in the PR, the problem here is that during combine
i2 pattern is substituted into more than one place in i3 pattern,
unique_copy is 0 (but, even if it would be non-zero, it could happen
if the comparison was processed first before the normal set inside of the
parallel) and thus the same RTL is (temporarily) shared between two
locations.  force_to_mode is first called with mask 0xdc36 (that is
actually find for both occurrences in the andhi_2 pattern) and later on
inside of the comparison again with mask 0x8000, and as it modifies
the IF_THEN_ELSE in place, it modifies also the other location (it is fine
if the comparison uses 0x8000 mask, but not in the other spot).
As in the end we fold it to a constant, we don't undo it and use incorrect
constant.

Fixed by making sure force_to_mode doesn't modify x in place.

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

2013-12-03  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/58726
	* combine.c (force_to_mode): Fix comment typo.  Don't destructively
	modify x for ROTATE, ROTATERT and IF_THEN_ELSE.

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


	Jakub
Jeff Law - Dec. 4, 2013, 4:43 a.m.
On 12/03/13 15:46, Jakub Jelinek wrote:
> Hi!
>
> As described in the PR, the problem here is that during combine
> i2 pattern is substituted into more than one place in i3 pattern,
> unique_copy is 0 (but, even if it would be non-zero, it could happen
> if the comparison was processed first before the normal set inside of the
> parallel) and thus the same RTL is (temporarily) shared between two
> locations.  force_to_mode is first called with mask 0xdc36 (that is
> actually find for both occurrences in the andhi_2 pattern) and later on
> inside of the comparison again with mask 0x8000, and as it modifies
> the IF_THEN_ELSE in place, it modifies also the other location (it is fine
> if the comparison uses 0x8000 mask, but not in the other spot).
> As in the end we fold it to a constant, we don't undo it and use incorrect
> constant.
>
> Fixed by making sure force_to_mode doesn't modify x in place.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
>
> 2013-12-03  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR rtl-optimization/58726
> 	* combine.c (force_to_mode): Fix comment typo.  Don't destructively
> 	modify x for ROTATE, ROTATERT and IF_THEN_ELSE.
>
> 	* gcc.c-torture/execute/pr58726.c: New test.
I'd worry there's other latent bugs of this nature and if we'd be better 
off avoiding the temporary sharing.  We have structure sharing rules for 
a reason -- I'd hate to think of all the code that would need auditing 
to ensure it was safe with this bogus sharing.

Any thoughts of how painful it'd be to avoid the sharing to start with?

jeff
Jakub Jelinek - Dec. 4, 2013, 8:03 a.m.
On Tue, Dec 03, 2013 at 09:43:51PM -0700, Jeff Law wrote:
> On 12/03/13 15:46, Jakub Jelinek wrote:
> >As described in the PR, the problem here is that during combine
> >i2 pattern is substituted into more than one place in i3 pattern,
> >unique_copy is 0 (but, even if it would be non-zero, it could happen
> >if the comparison was processed first before the normal set inside of the
> >parallel) and thus the same RTL is (temporarily) shared between two
> >locations.  force_to_mode is first called with mask 0xdc36 (that is
> >actually find for both occurrences in the andhi_2 pattern) and later on
> >inside of the comparison again with mask 0x8000, and as it modifies
> >the IF_THEN_ELSE in place, it modifies also the other location (it is fine
> >if the comparison uses 0x8000 mask, but not in the other spot).
> >As in the end we fold it to a constant, we don't undo it and use incorrect
> >constant.
> >
> >Fixed by making sure force_to_mode doesn't modify x in place.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
> >
> >2013-12-03  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR rtl-optimization/58726
> >	* combine.c (force_to_mode): Fix comment typo.  Don't destructively
> >	modify x for ROTATE, ROTATERT and IF_THEN_ELSE.
> >
> >	* gcc.c-torture/execute/pr58726.c: New test.
> I'd worry there's other latent bugs of this nature and if we'd be
> better off avoiding the temporary sharing.  We have structure
> sharing rules for a reason -- I'd hate to think of all the code that
> would need auditing to ensure it was safe with this bogus sharing.

I'm afraid I'm not familiar with the unwritten rules of combine.c enough
to know whether the above fix is all we need, or if there are further issues
just latent.

> Any thoughts of how painful it'd be to avoid the sharing to start with?

Perhaps most of the subst function could be moved to subst_1, drop the
and subst as a wrapper (with dropped unique_copy argument?) could do
for_each_rtx first to find out how many occurrences of from there are
in x, and if it is more than one, subst_1 then would copy_rtx for each
return of to other than the last one.  IMHO doing copy_rtx unconditionally
would be too expensive for the common case, would create too much garbage.

But it doesn't look like a safe thing to do on the release branches, at
least not until it is for a few months on the trunk.

	Jakub
Eric Botcazou - Dec. 4, 2013, 10:07 a.m.
> Fixed by making sure force_to_mode doesn't modify x in place.

I think that it's the way to go, force_to_mode doesn't modify its argument 
except for these 2 cases.  I'm not sure what the story is, but calling SUBST 
for these 2 cases doesn't seem really necessary.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
> 
> 2013-12-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/58726
> 	* combine.c (force_to_mode): Fix comment typo.  Don't destructively
> 	modify x for ROTATE, ROTATERT and IF_THEN_ELSE.
> 
> 	* gcc.c-torture/execute/pr58726.c: New test.

IMO it's the best fix at this point of the release cycles.
Eric Botcazou - Dec. 4, 2013, 10:23 a.m.
> I'd worry there's other latent bugs of this nature and if we'd be better
> off avoiding the temporary sharing.  We have structure sharing rules for
> a reason -- I'd hate to think of all the code that would need auditing
> to ensure it was safe with this bogus sharing.

I wouldn't throw the baby with the bath's water here, it's one of the numerous 
PRs opened by Zhendong Su and which clearly look machine-generated.  It fails 
with 4.4.x onwards and apparently nobody noticed the problem in real life.
We know that we have latent sharing issues in the combiner because of the way 
it's designed, but they are quite rare in practice.
Richard Guenther - Dec. 4, 2013, 10:40 a.m.
On Wed, Dec 4, 2013 at 11:07 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Fixed by making sure force_to_mode doesn't modify x in place.
>
> I think that it's the way to go, force_to_mode doesn't modify its argument
> except for these 2 cases.  I'm not sure what the story is, but calling SUBST
> for these 2 cases doesn't seem really necessary.
>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
>>
>> 2013-12-03  Jakub Jelinek  <jakub@redhat.com>
>>
>>       PR rtl-optimization/58726
>>       * combine.c (force_to_mode): Fix comment typo.  Don't destructively
>>       modify x for ROTATE, ROTATERT and IF_THEN_ELSE.
>>
>>       * gcc.c-torture/execute/pr58726.c: New test.
>
> IMO it's the best fix at this point of the release cycles.

I agree.

Richard.

> --
> Eric Botcazou
Jeff Law - Dec. 4, 2013, 4:20 p.m.
On 12/04/13 03:40, Richard Biener wrote:
> On Wed, Dec 4, 2013 at 11:07 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> Fixed by making sure force_to_mode doesn't modify x in place.
>>
>> I think that it's the way to go, force_to_mode doesn't modify its argument
>> except for these 2 cases.  I'm not sure what the story is, but calling SUBST
>> for these 2 cases doesn't seem really necessary.
>>
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8?
>>>
>>> 2013-12-03  Jakub Jelinek  <jakub@redhat.com>
>>>
>>>        PR rtl-optimization/58726
>>>        * combine.c (force_to_mode): Fix comment typo.  Don't destructively
>>>        modify x for ROTATE, ROTATERT and IF_THEN_ELSE.
>>>
>>>        * gcc.c-torture/execute/pr58726.c: New test.
>>
>> IMO it's the best fix at this point of the release cycles.
>
> I agree.
I can live with the nagging feeling that we've got a deeper problem here 
:-)  So I won't object to this approach.

jeff

Patch

--- gcc/combine.c.jj	2013-12-02 14:33:34.000000000 +0100
+++ gcc/combine.c	2013-12-03 20:37:25.140274452 +0100
@@ -8029,7 +8029,7 @@  force_to_mode (rtx x, enum machine_mode
   if (code == CALL || code == ASM_OPERANDS || code == CLOBBER)
     return x;
 
-  /* We want to perform the operation is its present mode unless we know
+  /* We want to perform the operation in its present mode unless we know
      that the operation is valid in MODE, in which case we do the operation
      in MODE.  */
   op_mode = ((GET_MODE_CLASS (mode) == GET_MODE_CLASS (GET_MODE (x))
@@ -8460,9 +8460,10 @@  force_to_mode (rtx x, enum machine_mode
 					    gen_int_mode (mask, GET_MODE (x)),
 					    XEXP (x, 1));
 	  if (temp && CONST_INT_P (temp))
-	    SUBST (XEXP (x, 0),
-		   force_to_mode (XEXP (x, 0), GET_MODE (x),
-				  INTVAL (temp), next_select));
+	    x = simplify_gen_binary (code, GET_MODE (x),
+				     force_to_mode (XEXP (x, 0), GET_MODE (x),
+						    INTVAL (temp), next_select),
+				     XEXP (x, 1));
 	}
       break;
 
@@ -8530,14 +8531,16 @@  force_to_mode (rtx x, enum machine_mode
       /* We have no way of knowing if the IF_THEN_ELSE can itself be
 	 written in a narrower mode.  We play it safe and do not do so.  */
 
-      SUBST (XEXP (x, 1),
-	     gen_lowpart_or_truncate (GET_MODE (x),
-				      force_to_mode (XEXP (x, 1), mode,
-						     mask, next_select)));
-      SUBST (XEXP (x, 2),
-	     gen_lowpart_or_truncate (GET_MODE (x),
-				      force_to_mode (XEXP (x, 2), mode,
-						     mask, next_select)));
+      op0 = gen_lowpart_or_truncate (GET_MODE (x),
+				     force_to_mode (XEXP (x, 1), mode,
+						    mask, next_select));
+      op1 = gen_lowpart_or_truncate (GET_MODE (x),
+				     force_to_mode (XEXP (x, 2), mode,
+						    mask, next_select));
+      if (op0 != XEXP (x, 1) || op1 != XEXP (x, 2))
+	x = simplify_gen_ternary (IF_THEN_ELSE, GET_MODE (x),
+				  GET_MODE (XEXP (x, 0)), XEXP (x, 0),
+				  op0, op1);
       break;
 
     default:
--- gcc/testsuite/gcc.c-torture/execute/pr58726.c.jj	2013-12-03 20:47:41.024094847 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr58726.c	2013-12-03 20:47:24.000000000 +0100
@@ -0,0 +1,26 @@ 
+/* PR rtl-optimization/58726 */
+
+int a, c;
+union { int f1; int f2 : 1; } b;
+
+short
+foo (short p)
+{
+  return p < 0 ? p : a;
+}
+
+int
+main ()
+{
+  if (sizeof (short) * __CHAR_BIT__ != 16
+      || sizeof (int) * __CHAR_BIT__ != 32)
+    return 0;
+  b.f1 = 56374;
+  unsigned short d;
+  int e = b.f2;
+  d = e == 0 ? b.f1 : 0;
+  c = foo (d);
+  if (c != (short) 56374)
+    __builtin_abort ();
+  return 0;
+}