* [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023)
@ 2011-11-09 16:14 Jakub Jelinek
2011-11-09 16:25 ` Jakub Jelinek
2011-11-09 16:47 ` Bernd Schmidt
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2011-11-09 16:14 UTC (permalink / raw)
To: bernds, ebotcazou; +Cc: gcc-patches
Hi!
This patch essentially reverts part of Bernd's 2011-07-06 changes,
which was IMHO wrong. As const_op here is a constant in wider mode than
MODE (which is the inner mode of the SIGN_EXTEND), the old code
(and what this patch is restoring) didn't check just that the sign bit
is clear, but also that all bits above the sign bit of MODE are clear too.
I've used GET_MODE_PRECISION instead of GET_MODE_BITSIZE (as changed
in many other places around that date) and HWI_COMPUTABLE_MODE_P macro.
Additionally, if GET_MODE_CLASS (mode) == MODE_INT, we know
mode != VOIDmode, there is no point testing it (but the compiler will
likely not know it, as GET_MODE_CLASS is reading from an array).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
BTW, the
@@ -6546,10 +6551,8 @@ simplify_set (rtx x)
enum machine_mode inner_mode = GET_MODE (inner);
/* Here we make sure that we don't have a sign bit on. */
- if (GET_MODE_BITSIZE (inner_mode) <= HOST_BITS_PER_WIDE_INT
- && (nonzero_bits (inner, inner_mode)
- < ((unsigned HOST_WIDE_INT) 1
- << (GET_MODE_BITSIZE (GET_MODE (src)) - 1))))
+ if (val_signbit_known_clear_p (GET_MODE (src),
+ nonzero_bits (inner, inner_mode)))
{
SUBST (SET_SRC (x), inner);
src = SET_SRC (x);
hunk from the same patch might have the same problem (can't nonzero_bits
be wider than the mode of src there?), but I don't have a testcase for that.
Can you please double check that?
2011-11-09 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/51023
* combine.c (simplify_comparison) <case SIGN_EXTEND>: Don't use
val_signbit_known_clear_p for signed comparison narrowing
optimization. Don't check for non-VOIDmode, use
HWI_COMPUTABLE_MODE_P macro.
<case ZERO_EXTEND>: Don't check for non-VOIDmode.
* gcc.c-torture/execute/pr51023.c: New test.
--- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
+++ gcc/combine.c 2011-11-09 10:06:27.207333364 +0100
@@ -11397,9 +11397,12 @@ simplify_comparison (enum rtx_code code,
later on, and then we wouldn't know whether to sign- or
zero-extend. */
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& ! unsigned_comparison_p
- && val_signbit_known_clear_p (mode, const_op)
+ && HWI_COMPUTABLE_MODE_P (mode)
+ && ((unsigned HOST_WIDE_INT) const_op
+ < (((unsigned HOST_WIDE_INT) 1
+ << (GET_MODE_PRECISION (mode) - 1))))
&& have_insn_for (COMPARE, mode))
{
op0 = XEXP (op0, 0);
@@ -11477,7 +11480,7 @@ simplify_comparison (enum rtx_code code,
case ZERO_EXTEND:
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& (unsigned_comparison_p || equality_comparison_p)
&& HWI_COMPUTABLE_MODE_P (mode)
&& ((unsigned HOST_WIDE_INT) const_op < GET_MODE_MASK (mode))
--- gcc/testsuite/gcc.c-torture/execute/pr51023.c.jj 2011-11-09 10:17:31.133406427 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr51023.c 2011-11-09 10:17:13.000000000 +0100
@@ -0,0 +1,18 @@
+/* PR rtl-optimization/51023 */
+
+extern void abort (void);
+
+short int
+foo (long int x)
+{
+ return x;
+}
+
+int
+main ()
+{
+ long int a = 0x4272AL;
+ if (foo (a) == a)
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023)
2011-11-09 16:14 [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023) Jakub Jelinek
@ 2011-11-09 16:25 ` Jakub Jelinek
2011-11-09 16:47 ` Bernd Schmidt
1 sibling, 0 replies; 7+ messages in thread
From: Jakub Jelinek @ 2011-11-09 16:25 UTC (permalink / raw)
To: Bernd Schmidt, Eric Botcazou; +Cc: gcc-patches
Hi!
This patch essentially reverts part of Bernd's 2011-07-06 changes,
which was IMHO wrong. As const_op here is a constant in wider mode than
MODE (which is the inner mode of the SIGN_EXTEND), the old code
(and what this patch is restoring) didn't check just that the sign bit
is clear, but also that all bits above the sign bit of MODE are clear too.
I've used GET_MODE_PRECISION instead of GET_MODE_BITSIZE (as changed
in many other places around that date) and HWI_COMPUTABLE_MODE_P macro.
Additionally, if GET_MODE_CLASS (mode) == MODE_INT, we know
mode != VOIDmode, there is no point testing it (but the compiler will
likely not know it, as GET_MODE_CLASS is reading from an array).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
BTW, the
@@ -6546,10 +6551,8 @@ simplify_set (rtx x)
enum machine_mode inner_mode = GET_MODE (inner);
/* Here we make sure that we don't have a sign bit on. */
- if (GET_MODE_BITSIZE (inner_mode) <= HOST_BITS_PER_WIDE_INT
- && (nonzero_bits (inner, inner_mode)
- < ((unsigned HOST_WIDE_INT) 1
- << (GET_MODE_BITSIZE (GET_MODE (src)) - 1))))
+ if (val_signbit_known_clear_p (GET_MODE (src),
+ nonzero_bits (inner, inner_mode)))
{
SUBST (SET_SRC (x), inner);
src = SET_SRC (x);
hunk from the same patch might have the same problem (can't nonzero_bits
be wider than the mode of src there?), but I don't have a testcase for that.
Can you please double check that?
2011-11-09 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/51023
* combine.c (simplify_comparison) <case SIGN_EXTEND>: Don't use
val_signbit_known_clear_p for signed comparison narrowing
optimization. Don't check for non-VOIDmode, use
HWI_COMPUTABLE_MODE_P macro.
<case ZERO_EXTEND>: Don't check for non-VOIDmode.
* gcc.c-torture/execute/pr51023.c: New test.
--- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
+++ gcc/combine.c 2011-11-09 10:06:27.207333364 +0100
@@ -11397,9 +11397,12 @@ simplify_comparison (enum rtx_code code,
later on, and then we wouldn't know whether to sign- or
zero-extend. */
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& ! unsigned_comparison_p
- && val_signbit_known_clear_p (mode, const_op)
+ && HWI_COMPUTABLE_MODE_P (mode)
+ && ((unsigned HOST_WIDE_INT) const_op
+ < (((unsigned HOST_WIDE_INT) 1
+ << (GET_MODE_PRECISION (mode) - 1))))
&& have_insn_for (COMPARE, mode))
{
op0 = XEXP (op0, 0);
@@ -11477,7 +11480,7 @@ simplify_comparison (enum rtx_code code,
case ZERO_EXTEND:
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& (unsigned_comparison_p || equality_comparison_p)
&& HWI_COMPUTABLE_MODE_P (mode)
&& ((unsigned HOST_WIDE_INT) const_op < GET_MODE_MASK (mode))
--- gcc/testsuite/gcc.c-torture/execute/pr51023.c.jj 2011-11-09 10:17:31.133406427 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr51023.c 2011-11-09 10:17:13.000000000 +0100
@@ -0,0 +1,18 @@
+/* PR rtl-optimization/51023 */
+
+extern void abort (void);
+
+short int
+foo (long int x)
+{
+ return x;
+}
+
+int
+main ()
+{
+ long int a = 0x4272AL;
+ if (foo (a) == a)
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023)
2011-11-09 16:14 [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023) Jakub Jelinek
2011-11-09 16:25 ` Jakub Jelinek
@ 2011-11-09 16:47 ` Bernd Schmidt
2011-11-09 17:09 ` Jakub Jelinek
1 sibling, 1 reply; 7+ messages in thread
From: Bernd Schmidt @ 2011-11-09 16:47 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 11/09/11 16:32, Jakub Jelinek wrote:
> --- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
> +++ gcc/combine.c 2011-11-09 10:06:27.207333364 +0100
> @@ -11397,9 +11397,12 @@ simplify_comparison (enum rtx_code code,
> later on, and then we wouldn't know whether to sign- or
> zero-extend. */
> mode = GET_MODE (XEXP (op0, 0));
> - if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
> + if (GET_MODE_CLASS (mode) == MODE_INT
> && ! unsigned_comparison_p
> - && val_signbit_known_clear_p (mode, const_op)
> + && HWI_COMPUTABLE_MODE_P (mode)
> + && ((unsigned HOST_WIDE_INT) const_op
> + < (((unsigned HOST_WIDE_INT) 1
> + << (GET_MODE_PRECISION (mode) - 1))))
> && have_insn_for (COMPARE, mode))
> {
> op0 = XEXP (op0, 0);
I guess this is OK, although it would still be nicer to use something
with a descriptive name rather than bit-fiddling, to avoid this kind of
confusion. Maybe check whether trunc_int_for_mode doesn't change the
constant? Or we could make val_signbit_known_clear_p do more extensive
checking.
Bernd
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023)
2011-11-09 16:47 ` Bernd Schmidt
@ 2011-11-09 17:09 ` Jakub Jelinek
2011-11-09 17:12 ` Bernd Schmidt
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2011-11-09 17:09 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
On Wed, Nov 09, 2011 at 04:44:55PM +0100, Bernd Schmidt wrote:
> On 11/09/11 16:32, Jakub Jelinek wrote:
> > --- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
> > +++ gcc/combine.c 2011-11-09 10:06:27.207333364 +0100
> > @@ -11397,9 +11397,12 @@ simplify_comparison (enum rtx_code code,
> > later on, and then we wouldn't know whether to sign- or
> > zero-extend. */
> > mode = GET_MODE (XEXP (op0, 0));
> > - if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
> > + if (GET_MODE_CLASS (mode) == MODE_INT
> > && ! unsigned_comparison_p
> > - && val_signbit_known_clear_p (mode, const_op)
> > + && HWI_COMPUTABLE_MODE_P (mode)
> > + && ((unsigned HOST_WIDE_INT) const_op
> > + < (((unsigned HOST_WIDE_INT) 1
> > + << (GET_MODE_PRECISION (mode) - 1))))
> > && have_insn_for (COMPARE, mode))
> > {
> > op0 = XEXP (op0, 0);
>
> I guess this is OK, although it would still be nicer to use something
> with a descriptive name rather than bit-fiddling, to avoid this kind of
> confusion. Maybe check whether trunc_int_for_mode doesn't change the
> constant? Or we could make val_signbit_known_clear_p do more extensive
> checking.
Do you think this is more readable?
That matches what the old code did, though I wonder if the c >= 0
test is needed there at all and if simply
&& trunc_int_for_mode (const_op, mode) == const_op
couldn't be used and handle also negative constants - the comparison
is signed and the other operand is sign extended...
2011-11-09 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/51023
* combine.c (simplify_comparison) <case SIGN_EXTEND>: Don't use
val_signbit_known_clear_p for signed comparison narrowing
optimization. Don't check for non-VOIDmode, use
HWI_COMPUTABLE_MODE_P macro.
<case ZERO_EXTEND>: Don't check for non-VOIDmode.
* gcc.c-torture/execute/pr51023.c: New test.
--- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
+++ gcc/combine.c 2011-11-09 10:06:27.207333364 +0100
@@ -11397,13 +11397,20 @@ simplify_comparison (enum rtx_code code,
later on, and then we wouldn't know whether to sign- or
zero-extend. */
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& ! unsigned_comparison_p
- && val_signbit_known_clear_p (mode, const_op)
- && have_insn_for (COMPARE, mode))
+ && HWI_COMPUTABLE_MODE_P (mode))
{
- op0 = XEXP (op0, 0);
- continue;
+ HOST_WIDE_INT c = trunc_int_for_mode (const_op, mode);
+ /* Don't do this if the sign bit of MODE or any
+ higher bit is set in CONST_OP. */
+ if (c == const_op
+ && c >= 0
+ && have_insn_for (COMPARE, mode))
+ {
+ op0 = XEXP (op0, 0);
+ continue;
+ }
}
break;
@@ -11477,7 +11484,7 @@ simplify_comparison (enum rtx_code code,
case ZERO_EXTEND:
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& (unsigned_comparison_p || equality_comparison_p)
&& HWI_COMPUTABLE_MODE_P (mode)
&& ((unsigned HOST_WIDE_INT) const_op < GET_MODE_MASK (mode))
--- gcc/testsuite/gcc.c-torture/execute/pr51023.c.jj 2011-11-09 10:17:31.133406427 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr51023.c 2011-11-09 10:17:13.000000000 +0100
@@ -0,0 +1,18 @@
+/* PR rtl-optimization/51023 */
+
+extern void abort (void);
+
+short int
+foo (long int x)
+{
+ return x;
+}
+
+int
+main ()
+{
+ long int a = 0x4272AL;
+ if (foo (a) == a)
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023)
2011-11-09 17:09 ` Jakub Jelinek
@ 2011-11-09 17:12 ` Bernd Schmidt
2011-11-09 17:21 ` [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023, take 3) Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Bernd Schmidt @ 2011-11-09 17:12 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 11/09/11 17:24, Jakub Jelinek wrote:
> --- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
> +++ gcc/combine.c 2011-11-09 10:06:27.207333364 +0100
> @@ -11397,13 +11397,20 @@ simplify_comparison (enum rtx_code code,
> later on, and then we wouldn't know whether to sign- or
> zero-extend. */
> mode = GET_MODE (XEXP (op0, 0));
> - if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
> + if (GET_MODE_CLASS (mode) == MODE_INT
> && ! unsigned_comparison_p
> - && val_signbit_known_clear_p (mode, const_op)
> - && have_insn_for (COMPARE, mode))
> + && HWI_COMPUTABLE_MODE_P (mode))
> {
> - op0 = XEXP (op0, 0);
> - continue;
> + HOST_WIDE_INT c = trunc_int_for_mode (const_op, mode);
> + /* Don't do this if the sign bit of MODE or any
> + higher bit is set in CONST_OP. */
> + if (c == const_op
> + && c >= 0
> + && have_insn_for (COMPARE, mode))
> + {
> + op0 = XEXP (op0, 0);
> + continue;
> + }
> }
Yes, I think I prefer this.
Bernd
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023, take 3)
2011-11-09 17:12 ` Bernd Schmidt
@ 2011-11-09 17:21 ` Jakub Jelinek
2011-11-10 16:06 ` Bernd Schmidt
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2011-11-09 17:21 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
On Wed, Nov 09, 2011 at 05:47:02PM +0100, Bernd Schmidt wrote:
> Yes, I think I prefer this.
So here is hopefully last iteration of that.
Negative constants that trunc_int_for_mode to the same value
are IMHO just fine too, similarly for ZERO_EXTEND 0xffff for HImode
should be fine too. On the other side, if mode is DImode and
outer mode of ZERO_EXTEND is TImode, if const_op had the highest bit
set, it would mean it is considered to be 65 1s followed by 63 other
bits. It is hard to construct testcases for these (except for
the failure), because apparently the FE is already narrowing the comparisons
if the constant is in range.
Ok for trunk if this bootstraps/regtests?
Can you please look at the simplify_set hunk? Thanks.
2011-11-09 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/51023
* combine.c (simplify_comparison) <case SIGN_EXTEND>: Don't use
val_signbit_known_clear_p for signed comparison narrowing
optimization. Don't check for non-VOIDmode, use
HWI_COMPUTABLE_MODE_P macro.
<case ZERO_EXTEND>: Don't check for non-VOIDmode.
Optimize even when const_op is equal to GET_MODE_MASK (mode),
don't optimize if const_op is negative.
* gcc.c-torture/execute/pr51023.c: New test.
--- gcc/combine.c.jj 2011-11-08 23:35:12.000000000 +0100
+++ gcc/combine.c 2011-11-09 18:03:54.185527804 +0100
@@ -11397,9 +11397,10 @@ simplify_comparison (enum rtx_code code,
later on, and then we wouldn't know whether to sign- or
zero-extend. */
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& ! unsigned_comparison_p
- && val_signbit_known_clear_p (mode, const_op)
+ && HWI_COMPUTABLE_MODE_P (mode)
+ && trunc_int_for_mode (const_op, mode) == const_op
&& have_insn_for (COMPARE, mode))
{
op0 = XEXP (op0, 0);
@@ -11477,10 +11478,11 @@ simplify_comparison (enum rtx_code code,
case ZERO_EXTEND:
mode = GET_MODE (XEXP (op0, 0));
- if (mode != VOIDmode && GET_MODE_CLASS (mode) == MODE_INT
+ if (GET_MODE_CLASS (mode) == MODE_INT
&& (unsigned_comparison_p || equality_comparison_p)
&& HWI_COMPUTABLE_MODE_P (mode)
- && ((unsigned HOST_WIDE_INT) const_op < GET_MODE_MASK (mode))
+ && (unsigned HOST_WIDE_INT) const_op <= GET_MODE_MASK (mode)
+ && const_op >= 0
&& have_insn_for (COMPARE, mode))
{
op0 = XEXP (op0, 0);
--- gcc/testsuite/gcc.c-torture/execute/pr51023.c.jj 2011-11-09 10:17:31.133406427 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr51023.c 2011-11-09 10:17:13.000000000 +0100
@@ -0,0 +1,18 @@
+/* PR rtl-optimization/51023 */
+
+extern void abort (void);
+
+short int
+foo (long int x)
+{
+ return x;
+}
+
+int
+main ()
+{
+ long int a = 0x4272AL;
+ if (foo (a) == a)
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023, take 3)
2011-11-09 17:21 ` [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023, take 3) Jakub Jelinek
@ 2011-11-10 16:06 ` Bernd Schmidt
0 siblings, 0 replies; 7+ messages in thread
From: Bernd Schmidt @ 2011-11-10 16:06 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 11/09/11 18:12, Jakub Jelinek wrote:
> So here is hopefully last iteration of that.
>
> Negative constants that trunc_int_for_mode to the same value
> are IMHO just fine too, similarly for ZERO_EXTEND 0xffff for HImode
> should be fine too. On the other side, if mode is DImode and
> outer mode of ZERO_EXTEND is TImode, if const_op had the highest bit
> set, it would mean it is considered to be 65 1s followed by 63 other
> bits. It is hard to construct testcases for these (except for
> the failure), because apparently the FE is already narrowing the comparisons
> if the constant is in range.
>
> Ok for trunk if this bootstraps/regtests?
Ok.
> Can you please look at the simplify_set hunk? Thanks.
That could safely just use the same kind of change, couldn't it?
Preapproved as well.
Bernd
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-11-10 14:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-09 16:14 [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023) Jakub Jelinek
2011-11-09 16:25 ` Jakub Jelinek
2011-11-09 16:47 ` Bernd Schmidt
2011-11-09 17:09 ` Jakub Jelinek
2011-11-09 17:12 ` Bernd Schmidt
2011-11-09 17:21 ` [PATCH] Fix combine's simplify_comparison (PR rtl-optimization/51023, take 3) Jakub Jelinek
2011-11-10 16:06 ` Bernd Schmidt
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).