* [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814]
@ 2022-03-13 10:32 Jakub Jelinek
2022-03-13 13:03 ` [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814] Eric Botcazou
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2022-03-13 10:32 UTC (permalink / raw)
To: Richard Biener, Jeff Law, Eric Botcazou; +Cc: gcc-patches
Hi!
find_if_case_{1,2} implicitly assumes conditional jumps and rewrites them,
so if they have extra side-effects or are say asm goto, things don't work
well, either the side-effects are lost or we could ICE.
In particular, the testcase below on s390x has there a doloop instruction
that decrements a register in addition to testing it for non-zero and
conditionally jumping based on that.
The following patch fixes that by punting for !onlyjump_p case, i.e.
if there are side-effects in the jump instruction or it isn't a plain PC
setter.
Bootstrapped/regtested on {x86_64,i686,powerpc64le,aarch64,s390x}-linux,
ok for trunk?
2022-03-13 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/104814
* ifcvt.cc (find_if_case_1, find_if_case_2): Punt if test_bb doesn't
end with onlyjump_p.
* gcc.c-torture/execute/pr104814.c: New test.
--- gcc/ifcvt.cc.jj 2022-02-09 12:55:50.750773751 +0100
+++ gcc/ifcvt.cc 2022-03-11 17:30:44.248855063 +0100
@@ -5259,6 +5259,10 @@ find_if_case_1 (basic_block test_bb, edg
&& CROSSING_JUMP_P (BB_END (else_bb))))
return FALSE;
+ /* Verify test_bb ends in a conditional jump with no other side-effects. */
+ if (!BB_END (test_bb) || !onlyjump_p (BB_END (test_bb)))
+ return FALSE;
+
/* THEN has one successor. */
if (!single_succ_p (then_bb))
return FALSE;
@@ -5380,6 +5384,10 @@ find_if_case_2 (basic_block test_bb, edg
&& CROSSING_JUMP_P (BB_END (else_bb))))
return FALSE;
+ /* Verify test_bb ends in a conditional jump with no other side-effects. */
+ if (!BB_END (test_bb) || !onlyjump_p (BB_END (test_bb)))
+ return FALSE;
+
/* ELSE has one successor. */
if (!single_succ_p (else_bb))
return FALSE;
--- gcc/testsuite/gcc.c-torture/execute/pr104814.c.jj 2022-03-11 17:36:58.753651766 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr104814.c 2022-03-11 17:36:45.120841179 +0100
@@ -0,0 +1,30 @@
+/* PR rtl-optimization/104814 */
+
+short a = 0;
+static long b = 0;
+int c = 7;
+char d = 0;
+short *e = &a;
+long f = 0;
+
+unsigned long
+foo (unsigned long h, long j)
+{
+ return j == 0 ? h : h / j;
+}
+
+int
+main ()
+{
+ long k = f;
+ for (; c; --c)
+ {
+ for (int i = 0; i < 7; ++i)
+ ;
+ long m = foo (f, --b);
+ d = ((char) m | *e) <= 43165;
+ }
+ if (b != -7)
+ __builtin_abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814]
2022-03-13 10:32 [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814] Jakub Jelinek
@ 2022-03-13 13:03 ` Eric Botcazou
2022-03-13 13:43 ` [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814] Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Eric Botcazou @ 2022-03-13 13:03 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, Jeff Law, gcc-patches
> Bootstrapped/regtested on {x86_64,i686,powerpc64le,aarch64,s390x}-linux,
> ok for trunk?
>
> 2022-03-13 Jakub Jelinek <jakub@redhat.com>
>
> PR rtl-optimization/104814
> * ifcvt.cc (find_if_case_1, find_if_case_2): Punt if test_bb
> doesn't end with onlyjump_p.
>
> * gcc.c-torture/execute/pr104814.c: New test.
>
> --- gcc/ifcvt.cc.jj 2022-02-09 12:55:50.750773751 +0100
> +++ gcc/ifcvt.cc 2022-03-11 17:30:44.248855063 +0100
> @@ -5259,6 +5259,10 @@ find_if_case_1 (basic_block test_bb, edg
> && CROSSING_JUMP_P (BB_END (else_bb))))
> return FALSE;
>
> + /* Verify test_bb ends in a conditional jump with no other side-effects.
> */ + if (!BB_END (test_bb) || !onlyjump_p (BB_END (test_bb)))
> + return FALSE;
> +
> /* THEN has one successor. */
> if (!single_succ_p (then_bb))
> return FALSE;
> @@ -5380,6 +5384,10 @@ find_if_case_2 (basic_block test_bb, edg
> && CROSSING_JUMP_P (BB_END (else_bb))))
> return FALSE;
>
> + /* Verify test_bb ends in a conditional jump with no other side-effects.
> */ + if (!BB_END (test_bb) || !onlyjump_p (BB_END (test_bb)))
> + return FALSE;
> +
> /* ELSE has one successor. */
> if (!single_succ_p (else_bb))
> return FALSE;
Are the !BB_END tests really necessary? cond_exec_process_if_block has the
same test on onlyjump_p without it. Likewise for noce_find_if_block.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814]
2022-03-13 13:03 ` [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814] Eric Botcazou
@ 2022-03-13 13:43 ` Jakub Jelinek
2022-03-14 10:24 ` [PATCH] ifcvt, v2: " Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2022-03-13 13:43 UTC (permalink / raw)
To: Eric Botcazou; +Cc: Richard Biener, Jeff Law, gcc-patches
On Sun, Mar 13, 2022 at 02:03:33PM +0100, Eric Botcazou wrote:
> > Bootstrapped/regtested on {x86_64,i686,powerpc64le,aarch64,s390x}-linux,
> > ok for trunk?
> >
> > 2022-03-13 Jakub Jelinek <jakub@redhat.com>
> >
> > PR rtl-optimization/104814
> > * ifcvt.cc (find_if_case_1, find_if_case_2): Punt if test_bb
> > doesn't end with onlyjump_p.
> >
> > * gcc.c-torture/execute/pr104814.c: New test.
> >
> > --- gcc/ifcvt.cc.jj 2022-02-09 12:55:50.750773751 +0100
> > +++ gcc/ifcvt.cc 2022-03-11 17:30:44.248855063 +0100
> > @@ -5259,6 +5259,10 @@ find_if_case_1 (basic_block test_bb, edg
> > && CROSSING_JUMP_P (BB_END (else_bb))))
> > return FALSE;
> >
> > + /* Verify test_bb ends in a conditional jump with no other side-effects.
> > */ + if (!BB_END (test_bb) || !onlyjump_p (BB_END (test_bb)))
> > + return FALSE;
> > +
> > /* THEN has one successor. */
> > if (!single_succ_p (then_bb))
> > return FALSE;
> > @@ -5380,6 +5384,10 @@ find_if_case_2 (basic_block test_bb, edg
> > && CROSSING_JUMP_P (BB_END (else_bb))))
> > return FALSE;
> >
> > + /* Verify test_bb ends in a conditional jump with no other side-effects.
> > */ + if (!BB_END (test_bb) || !onlyjump_p (BB_END (test_bb)))
> > + return FALSE;
> > +
> > /* ELSE has one successor. */
> > if (!single_succ_p (else_bb))
> > return FALSE;
>
> Are the !BB_END tests really necessary? cond_exec_process_if_block has the
> same test on onlyjump_p without it. Likewise for noce_find_if_block.
Probably not, I've put it there only because the neighboring code is testing
it too:
if ((BB_END (then_bb)
&& JUMP_P (BB_END (then_bb))
&& CROSSING_JUMP_P (BB_END (then_bb)))
|| (BB_END (test_bb)
&& JUMP_P (BB_END (test_bb))
&& CROSSING_JUMP_P (BB_END (test_bb)))
|| (BB_END (else_bb)
&& JUMP_P (BB_END (else_bb))
&& CROSSING_JUMP_P (BB_END (else_bb))))
return FALSE;
find_if_header which calls find_if_case_* has:
if (EDGE_COUNT (test_bb->succs) != 2)
return NULL;
at the start, and I think an empty bb can't have more than one successor,
because there is nothing to cause different control flow.
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ifcvt, v2: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814]
2022-03-13 13:43 ` [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814] Jakub Jelinek
@ 2022-03-14 10:24 ` Jakub Jelinek
2022-03-14 10:30 ` [PATCH] ifcvt, v2: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814] Eric Botcazou
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2022-03-14 10:24 UTC (permalink / raw)
To: Eric Botcazou, gcc-patches, Richard Biener
On Sun, Mar 13, 2022 at 02:43:18PM +0100, Jakub Jelinek via Gcc-patches wrote:
> > Are the !BB_END tests really necessary? cond_exec_process_if_block has the
> > same test on onlyjump_p without it. Likewise for noce_find_if_block.
>
> Probably not, I've put it there only because the neighboring code is testing
> it too:
> if ((BB_END (then_bb)
> && JUMP_P (BB_END (then_bb))
> && CROSSING_JUMP_P (BB_END (then_bb)))
> || (BB_END (test_bb)
> && JUMP_P (BB_END (test_bb))
> && CROSSING_JUMP_P (BB_END (test_bb)))
> || (BB_END (else_bb)
> && JUMP_P (BB_END (else_bb))
> && CROSSING_JUMP_P (BB_END (else_bb))))
> return FALSE;
> find_if_header which calls find_if_case_* has:
> if (EDGE_COUNT (test_bb->succs) != 2)
> return NULL;
> at the start, and I think an empty bb can't have more than one successor,
> because there is nothing to cause different control flow.
Here is an updated patch for it. Ok for trunk?
2022-03-14 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/104814
* ifcvt.cc (find_if_case_1, find_if_case_2): Punt if test_bb doesn't
end with onlyjump_p. Assume BB_END (test_bb) is always non-NULL.
* gcc.c-torture/execute/pr104814.c: New test.
--- gcc/ifcvt.cc.jj 2022-03-14 10:34:16.350172371 +0100
+++ gcc/ifcvt.cc 2022-03-14 11:20:32.425384101 +0100
@@ -5251,14 +5251,17 @@ find_if_case_1 (basic_block test_bb, edg
if ((BB_END (then_bb)
&& JUMP_P (BB_END (then_bb))
&& CROSSING_JUMP_P (BB_END (then_bb)))
- || (BB_END (test_bb)
- && JUMP_P (BB_END (test_bb))
+ || (JUMP_P (BB_END (test_bb))
&& CROSSING_JUMP_P (BB_END (test_bb)))
|| (BB_END (else_bb)
&& JUMP_P (BB_END (else_bb))
&& CROSSING_JUMP_P (BB_END (else_bb))))
return FALSE;
+ /* Verify test_bb ends in a conditional jump with no other side-effects. */
+ if (!onlyjump_p (BB_END (test_bb)))
+ return FALSE;
+
/* THEN has one successor. */
if (!single_succ_p (then_bb))
return FALSE;
@@ -5372,14 +5375,17 @@ find_if_case_2 (basic_block test_bb, edg
if ((BB_END (then_bb)
&& JUMP_P (BB_END (then_bb))
&& CROSSING_JUMP_P (BB_END (then_bb)))
- || (BB_END (test_bb)
- && JUMP_P (BB_END (test_bb))
+ || (JUMP_P (BB_END (test_bb))
&& CROSSING_JUMP_P (BB_END (test_bb)))
|| (BB_END (else_bb)
&& JUMP_P (BB_END (else_bb))
&& CROSSING_JUMP_P (BB_END (else_bb))))
return FALSE;
+ /* Verify test_bb ends in a conditional jump with no other side-effects. */
+ if (!onlyjump_p (BB_END (test_bb)))
+ return FALSE;
+
/* ELSE has one successor. */
if (!single_succ_p (else_bb))
return FALSE;
--- gcc/testsuite/gcc.c-torture/execute/pr104814.c.jj 2022-03-14 11:18:57.188717248 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr104814.c 2022-03-14 11:18:57.188717248 +0100
@@ -0,0 +1,30 @@
+/* PR rtl-optimization/104814 */
+
+short a = 0;
+static long b = 0;
+int c = 7;
+char d = 0;
+short *e = &a;
+long f = 0;
+
+unsigned long
+foo (unsigned long h, long j)
+{
+ return j == 0 ? h : h / j;
+}
+
+int
+main ()
+{
+ long k = f;
+ for (; c; --c)
+ {
+ for (int i = 0; i < 7; ++i)
+ ;
+ long m = foo (f, --b);
+ d = ((char) m | *e) <= 43165;
+ }
+ if (b != -7)
+ __builtin_abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ifcvt, v2: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814]
2022-03-14 10:24 ` [PATCH] ifcvt, v2: " Jakub Jelinek
@ 2022-03-14 10:30 ` Eric Botcazou
0 siblings, 0 replies; 5+ messages in thread
From: Eric Botcazou @ 2022-03-14 10:30 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Richard Biener
> Here is an updated patch for it. Ok for trunk?
>
> 2022-03-14 Jakub Jelinek <jakub@redhat.com>
>
> PR rtl-optimization/104814
> * ifcvt.cc (find_if_case_1, find_if_case_2): Punt if test_bb doesn't
> end with onlyjump_p. Assume BB_END (test_bb) is always non-NULL.
>
> * gcc.c-torture/execute/pr104814.c: New test.
OK, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-14 10:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-13 10:32 [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814] Jakub Jelinek
2022-03-13 13:03 ` [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814] Eric Botcazou
2022-03-13 13:43 ` [PATCH] ifcvt: Punt if not onlyjump_p for find_if_case_{1,2} [PR104814] Jakub Jelinek
2022-03-14 10:24 ` [PATCH] ifcvt, v2: " Jakub Jelinek
2022-03-14 10:30 ` [PATCH] ifcvt, v2: Punt if not onlyjump_p for find_if_case_{1, 2} [PR104814] Eric Botcazou
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).