| Message ID | 20131203224654.GH892@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
> 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'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.
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
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
--- 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; +}