* [PATCH] Fix combiner issues with shifts (PR c/37924)
@ 2008-10-27 20:58 Jakub Jelinek
2008-10-27 21:54 ` Michael Meissner
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2008-10-27 20:58 UTC (permalink / raw)
To: gcc-patches
Hi!
This patch fixes a couple of combiner issues.
1) simplify_shift_const_1 does something only if a shift count is within
its range, but then can add or subtract shift counts without checking the
result. This is because each such addition or subtraction is followed by
continue and expects next iteration to canonicalize the count.
Unfortunately if complement_p is true, the loop is exited before that
canonicalization. This patch moves if (complement_p) break; after
the shift count canonicalization.
2) make_compound_operation in several places didn't check if shift count is
in its range, and calling make_extraction with negative length results in
ICE. We should leave out-of-range shift counts unmodified.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2008-10-27 Jakub Jelinek <jakub@redhat.com>
PR c/37924
* combine.c (make_compound_operation): Don't call make_extraction with
non-positive length.
(simplify_shift_const_1): Canonicalize count even if complement_p.
* gcc.c-torture/execute/pr37924.c: New test.
--- gcc/combine.c.jj 2008-10-23 13:21:41.000000000 +0200
+++ gcc/combine.c 2008-10-27 17:05:29.000000000 +0100
@@ -6996,7 +6996,8 @@ make_compound_operation (rtx x, enum rtx
if (GET_CODE (rhs) == CONST_INT
&& GET_CODE (lhs) == ASHIFT
&& GET_CODE (XEXP (lhs, 1)) == CONST_INT
- && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1)))
+ && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1))
+ && INTVAL (rhs) < mode_width)
{
new_rtx = make_compound_operation (XEXP (lhs, 0), next_code);
new_rtx = make_extraction (mode, new_rtx,
@@ -7016,6 +7017,7 @@ make_compound_operation (rtx x, enum rtx
&& (OBJECT_P (SUBREG_REG (lhs))))
&& GET_CODE (rhs) == CONST_INT
&& INTVAL (rhs) < HOST_BITS_PER_WIDE_INT
+ && INTVAL (rhs) < mode_width
&& (new_rtx = extract_left_shift (lhs, INTVAL (rhs))) != 0)
new_rtx = make_extraction (mode, make_compound_operation (new_rtx, next_code),
0, NULL_RTX, mode_width - INTVAL (rhs),
@@ -9003,11 +9005,6 @@ simplify_shift_const_1 (enum rtx_code co
if (GET_CODE (varop) == CLOBBER)
return NULL_RTX;
- /* If we discovered we had to complement VAROP, leave. Making a NOT
- here would cause an infinite loop. */
- if (complement_p)
- break;
-
/* Convert ROTATERT to ROTATE. */
if (code == ROTATERT)
{
@@ -9053,6 +9050,11 @@ simplify_shift_const_1 (enum rtx_code co
}
}
+ /* If we discovered we had to complement VAROP, leave. Making a NOT
+ here would cause an infinite loop. */
+ if (complement_p)
+ break;
+
/* An arithmetic right shift of a quantity known to be -1 or 0
is a no-op. */
if (code == ASHIFTRT
--- gcc/testsuite/gcc.c-torture/execute/pr37924.c.jj 2008-10-27 19:24:11.000000000 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr37924.c 2008-10-27 19:23:10.000000000 +0100
@@ -0,0 +1,29 @@
+/* PR c/37924 */
+
+extern void abort (void);
+
+char a;
+
+int
+test (void)
+{
+ int b = -1;
+ return ((unsigned int) (a ^ b)) >> 9;
+}
+
+int
+main (void)
+{
+ if (test () != (-1U >> 9))
+ abort ();
+ a = 0x40;
+ if (test () != (-1U >> 9))
+ abort ();
+ a = 0x80;
+ if (test () != (a < 0) ? 0 : (-1U >> 9))
+ abort ();
+ a = 0xff;
+ if (test () != (a < 0) ? 0 : (-1U >> 9))
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix combiner issues with shifts (PR c/37924)
2008-10-27 20:58 [PATCH] Fix combiner issues with shifts (PR c/37924) Jakub Jelinek
@ 2008-10-27 21:54 ` Michael Meissner
2008-10-27 22:30 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Michael Meissner @ 2008-10-27 21:54 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Mon, Oct 27, 2008 at 09:02:07PM +0100, Jakub Jelinek wrote:
> Hi!
>
> This patch fixes a couple of combiner issues.
> 1) simplify_shift_const_1 does something only if a shift count is within
> its range, but then can add or subtract shift counts without checking the
> result. This is because each such addition or subtraction is followed by
> continue and expects next iteration to canonicalize the count.
> Unfortunately if complement_p is true, the loop is exited before that
> canonicalization. This patch moves if (complement_p) break; after
> the shift count canonicalization.
> 2) make_compound_operation in several places didn't check if shift count is
> in its range, and calling make_extraction with negative length results in
> ICE. We should leave out-of-range shift counts unmodified.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
The code looks correct to me. However, your test code should declare a to be
signed char instead of char, since there are platforms where chars are unsigned
by default.
> 2008-10-27 Jakub Jelinek <jakub@redhat.com>
>
> PR c/37924
> * combine.c (make_compound_operation): Don't call make_extraction with
> non-positive length.
> (simplify_shift_const_1): Canonicalize count even if complement_p.
>
> * gcc.c-torture/execute/pr37924.c: New test.
>
> --- gcc/combine.c.jj 2008-10-23 13:21:41.000000000 +0200
> +++ gcc/combine.c 2008-10-27 17:05:29.000000000 +0100
> @@ -6996,7 +6996,8 @@ make_compound_operation (rtx x, enum rtx
> if (GET_CODE (rhs) == CONST_INT
> && GET_CODE (lhs) == ASHIFT
> && GET_CODE (XEXP (lhs, 1)) == CONST_INT
> - && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1)))
> + && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1))
> + && INTVAL (rhs) < mode_width)
> {
> new_rtx = make_compound_operation (XEXP (lhs, 0), next_code);
> new_rtx = make_extraction (mode, new_rtx,
> @@ -7016,6 +7017,7 @@ make_compound_operation (rtx x, enum rtx
> && (OBJECT_P (SUBREG_REG (lhs))))
> && GET_CODE (rhs) == CONST_INT
> && INTVAL (rhs) < HOST_BITS_PER_WIDE_INT
> + && INTVAL (rhs) < mode_width
> && (new_rtx = extract_left_shift (lhs, INTVAL (rhs))) != 0)
> new_rtx = make_extraction (mode, make_compound_operation (new_rtx, next_code),
> 0, NULL_RTX, mode_width - INTVAL (rhs),
> @@ -9003,11 +9005,6 @@ simplify_shift_const_1 (enum rtx_code co
> if (GET_CODE (varop) == CLOBBER)
> return NULL_RTX;
>
> - /* If we discovered we had to complement VAROP, leave. Making a NOT
> - here would cause an infinite loop. */
> - if (complement_p)
> - break;
> -
> /* Convert ROTATERT to ROTATE. */
> if (code == ROTATERT)
> {
> @@ -9053,6 +9050,11 @@ simplify_shift_const_1 (enum rtx_code co
> }
> }
>
> + /* If we discovered we had to complement VAROP, leave. Making a NOT
> + here would cause an infinite loop. */
> + if (complement_p)
> + break;
> +
> /* An arithmetic right shift of a quantity known to be -1 or 0
> is a no-op. */
> if (code == ASHIFTRT
> --- gcc/testsuite/gcc.c-torture/execute/pr37924.c.jj 2008-10-27 19:24:11.000000000 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr37924.c 2008-10-27 19:23:10.000000000 +0100
> @@ -0,0 +1,29 @@
> +/* PR c/37924 */
> +
> +extern void abort (void);
> +
> +char a;
> +
> +int
> +test (void)
> +{
> + int b = -1;
> + return ((unsigned int) (a ^ b)) >> 9;
> +}
> +
> +int
> +main (void)
> +{
> + if (test () != (-1U >> 9))
> + abort ();
> + a = 0x40;
> + if (test () != (-1U >> 9))
> + abort ();
> + a = 0x80;
> + if (test () != (a < 0) ? 0 : (-1U >> 9))
> + abort ();
> + a = 0xff;
> + if (test () != (a < 0) ? 0 : (-1U >> 9))
> + abort ();
> + return 0;
> +}
>
> Jakub
--
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix combiner issues with shifts (PR c/37924)
2008-10-27 21:54 ` Michael Meissner
@ 2008-10-27 22:30 ` Jakub Jelinek
2008-10-27 22:47 ` Michael Meissner
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2008-10-27 22:30 UTC (permalink / raw)
To: Michael Meissner, gcc-patches
On Mon, Oct 27, 2008 at 05:13:20PM -0400, Michael Meissner wrote:
> However, your test code should declare a to be
> signed char instead of char, since there are platforms where chars are unsigned
> by default.
I'm aware of that, that's why I've tested the testcase works with both
-fsigned-char and -funsigned-char. I wanted to cover both signed char
(on some arches) and unsigned char (on others).
> > --- gcc/testsuite/gcc.c-torture/execute/pr37924.c.jj 2008-10-27 19:24:11.000000000 +0100
> > +++ gcc/testsuite/gcc.c-torture/execute/pr37924.c 2008-10-27 19:23:10.000000000 +0100
> > @@ -0,0 +1,29 @@
> > +/* PR c/37924 */
> > +
> > +extern void abort (void);
> > +
> > +char a;
> > +
> > +int
> > +test (void)
> > +{
> > + int b = -1;
> > + return ((unsigned int) (a ^ b)) >> 9;
> > +}
> > +
> > +int
> > +main (void)
> > +{
> > + if (test () != (-1U >> 9))
> > + abort ();
> > + a = 0x40;
> > + if (test () != (-1U >> 9))
> > + abort ();
> > + a = 0x80;
> > + if (test () != (a < 0) ? 0 : (-1U >> 9))
> > + abort ();
> > + a = 0xff;
> > + if (test () != (a < 0) ? 0 : (-1U >> 9))
> > + abort ();
> > + return 0;
> > +}
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix combiner issues with shifts (PR c/37924)
2008-10-27 22:30 ` Jakub Jelinek
@ 2008-10-27 22:47 ` Michael Meissner
2008-10-28 12:38 ` [PATCH] Fix combiner issues with shifts (PR c/37924, take 2) Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Michael Meissner @ 2008-10-27 22:47 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Michael Meissner, gcc-patches
On Mon, Oct 27, 2008 at 10:22:42PM +0100, Jakub Jelinek wrote:
> On Mon, Oct 27, 2008 at 05:13:20PM -0400, Michael Meissner wrote:
> > However, your test code should declare a to be
> > signed char instead of char, since there are platforms where chars are unsigned
> > by default.
>
> I'm aware of that, that's why I've tested the testcase works with both
> -fsigned-char and -funsigned-char. I wanted to cover both signed char
> (on some arches) and unsigned char (on others).
I was just picking up on whether storing 0x80 would someday cause the test to
fail on different signed/unsigned char configurations (since 0x80 can't be fit
in a normal char), and I figured it was better to write type correct code to
have the meaning clear.
--
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Fix combiner issues with shifts (PR c/37924, take 2)
2008-10-27 22:47 ` Michael Meissner
@ 2008-10-28 12:38 ` Jakub Jelinek
2008-10-28 21:43 ` Michael Meissner
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2008-10-28 12:38 UTC (permalink / raw)
To: Michael Meissner; +Cc: gcc-patches
On Mon, Oct 27, 2008 at 05:26:38PM -0400, Michael Meissner wrote:
> On Mon, Oct 27, 2008 at 10:22:42PM +0100, Jakub Jelinek wrote:
> > On Mon, Oct 27, 2008 at 05:13:20PM -0400, Michael Meissner wrote:
> > > However, your test code should declare a to be
> > > signed char instead of char, since there are platforms where chars are unsigned
> > > by default.
> >
> > I'm aware of that, that's why I've tested the testcase works with both
> > -fsigned-char and -funsigned-char. I wanted to cover both signed char
> > (on some arches) and unsigned char (on others).
>
> I was just picking up on whether storing 0x80 would someday cause the test to
> fail on different signed/unsigned char configurations (since 0x80 can't be fit
> in a normal char), and I figured it was better to write type correct code to
> have the meaning clear.
Ok, here is the same patch with testcase that tests both signed char and
unsigned char on all targets. Bootstrapped/regtested on x86_64-linux, ok
for trunk?
2008-10-28 Jakub Jelinek <jakub@redhat.com>
PR c/37924
* combine.c (make_compound_operation): Don't call make_extraction with
non-positive length.
(simplify_shift_const_1): Canonicalize count even if complement_p.
* gcc.c-torture/execute/pr37924.c: New test.
--- gcc/combine.c.jj 2008-10-14 12:49:27.000000000 +0200
+++ gcc/combine.c 2008-10-27 19:33:25.000000000 +0100
@@ -6996,7 +6996,8 @@ make_compound_operation (rtx x, enum rtx
if (GET_CODE (rhs) == CONST_INT
&& GET_CODE (lhs) == ASHIFT
&& GET_CODE (XEXP (lhs, 1)) == CONST_INT
- && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1)))
+ && INTVAL (rhs) >= INTVAL (XEXP (lhs, 1))
+ && INTVAL (rhs) < mode_width)
{
new_rtx = make_compound_operation (XEXP (lhs, 0), next_code);
new_rtx = make_extraction (mode, new_rtx,
@@ -7016,6 +7017,7 @@ make_compound_operation (rtx x, enum rtx
&& (OBJECT_P (SUBREG_REG (lhs))))
&& GET_CODE (rhs) == CONST_INT
&& INTVAL (rhs) < HOST_BITS_PER_WIDE_INT
+ && INTVAL (rhs) < mode_width
&& (new_rtx = extract_left_shift (lhs, INTVAL (rhs))) != 0)
new_rtx = make_extraction (mode, make_compound_operation (new_rtx, next_code),
0, NULL_RTX, mode_width - INTVAL (rhs),
@@ -9003,11 +9005,6 @@ simplify_shift_const_1 (enum rtx_code co
if (GET_CODE (varop) == CLOBBER)
return NULL_RTX;
- /* If we discovered we had to complement VAROP, leave. Making a NOT
- here would cause an infinite loop. */
- if (complement_p)
- break;
-
/* Convert ROTATERT to ROTATE. */
if (code == ROTATERT)
{
@@ -9053,6 +9050,11 @@ simplify_shift_const_1 (enum rtx_code co
}
}
+ /* If we discovered we had to complement VAROP, leave. Making a NOT
+ here would cause an infinite loop. */
+ if (complement_p)
+ break;
+
/* An arithmetic right shift of a quantity known to be -1 or 0
is a no-op. */
if (code == ASHIFTRT
--- gcc/testsuite/gcc.c-torture/execute/pr37924.c.jj 2008-10-27 19:33:25.000000000 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr37924.c 2008-10-28 10:35:39.000000000 +0100
@@ -0,0 +1,50 @@
+/* PR c/37924 */
+
+extern void abort (void);
+
+signed char a;
+unsigned char b;
+
+int
+test1 (void)
+{
+ int c = -1;
+ return ((unsigned int) (a ^ c)) >> 9;
+}
+
+int
+test2 (void)
+{
+ int c = -1;
+ return ((unsigned int) (b ^ c)) >> 9;
+}
+
+int
+main (void)
+{
+ a = 0;
+ if (test1 () != (-1U >> 9))
+ abort ();
+ a = 0x40;
+ if (test1 () != (-1U >> 9))
+ abort ();
+ a = 0x80;
+ if (test1 () != (a < 0) ? 0 : (-1U >> 9))
+ abort ();
+ a = 0xff;
+ if (test1 () != (a < 0) ? 0 : (-1U >> 9))
+ abort ();
+ b = 0;
+ if (test2 () != (-1U >> 9))
+ abort ();
+ b = 0x40;
+ if (test2 () != (-1U >> 9))
+ abort ();
+ b = 0x80;
+ if (test2 () != (-1U >> 9))
+ abort ();
+ b = 0xff;
+ if (test2 () != (-1U >> 9))
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix combiner issues with shifts (PR c/37924, take 2)
2008-10-28 12:38 ` [PATCH] Fix combiner issues with shifts (PR c/37924, take 2) Jakub Jelinek
@ 2008-10-28 21:43 ` Michael Meissner
0 siblings, 0 replies; 6+ messages in thread
From: Michael Meissner @ 2008-10-28 21:43 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Michael Meissner, gcc-patches
On Tue, Oct 28, 2008 at 12:27:18PM +0100, Jakub Jelinek wrote:
> On Mon, Oct 27, 2008 at 05:26:38PM -0400, Michael Meissner wrote:
> > On Mon, Oct 27, 2008 at 10:22:42PM +0100, Jakub Jelinek wrote:
> > > On Mon, Oct 27, 2008 at 05:13:20PM -0400, Michael Meissner wrote:
> > > > However, your test code should declare a to be
> > > > signed char instead of char, since there are platforms where chars are unsigned
> > > > by default.
> > >
> > > I'm aware of that, that's why I've tested the testcase works with both
> > > -fsigned-char and -funsigned-char. I wanted to cover both signed char
> > > (on some arches) and unsigned char (on others).
> >
> > I was just picking up on whether storing 0x80 would someday cause the test to
> > fail on different signed/unsigned char configurations (since 0x80 can't be fit
> > in a normal char), and I figured it was better to write type correct code to
> > have the meaning clear.
>
> Ok, here is the same patch with testcase that tests both signed char and
> unsigned char on all targets. Bootstrapped/regtested on x86_64-linux, ok
> for trunk?
Ok.
--
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-10-28 18:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-27 20:58 [PATCH] Fix combiner issues with shifts (PR c/37924) Jakub Jelinek
2008-10-27 21:54 ` Michael Meissner
2008-10-27 22:30 ` Jakub Jelinek
2008-10-27 22:47 ` Michael Meissner
2008-10-28 12:38 ` [PATCH] Fix combiner issues with shifts (PR c/37924, take 2) Jakub Jelinek
2008-10-28 21:43 ` Michael Meissner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).