* [PATCH] combine: Fix split_i2i3 ICE [PR94291]
@ 2020-03-31 7:04 Jakub Jelinek
2020-04-07 10:53 ` Patch ping: " Jakub Jelinek
2020-04-07 19:18 ` Segher Boessenkool
0 siblings, 2 replies; 3+ messages in thread
From: Jakub Jelinek @ 2020-03-31 7:04 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
Hi!
The following testcase ICEs on armv7hl-linux-gnueabi.
try_combine is called on:
(gdb) p debug_rtx (i3)
(insn 20 12 22 2 (set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
(const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
(reg:SI 125)) "pr94291.c":7:8 241 {*arm_movsi_insn}
(expr_list:REG_DEAD (reg:SI 125)
(nil)))
(gdb) p debug_rtx (i2)
(insn 12 7 20 2 (parallel [
(set (reg:CC 100 cc)
(compare:CC (reg:SI 121 [ <retval> ])
(const_int 0 [0])))
(set (reg:SI 125)
(reg:SI 121 [ <retval> ]))
]) "pr94291.c":7:8 248 {*movsi_compare0}
(expr_list:REG_UNUSED (reg:CC 100 cc)
(nil)))
and tries to recognize cc = r121 cmp 0; [sfp-4] = r121 parallel,
but that isn't recognized, so it splits it into two: split_i2i3
[sfp-4] = r121 followed by cc = r121 cmp 0 which is recognized, but
ICEs because the code below insist that the SET_DEST of newi2pat
(or first set in PARALLEL thereof) must be a REG or SUBREG of REG,
but it is a MEM in this case. I don't see any condition that would
guarantee that, perhaps for the swap_i2i3 case it was somehow guaranteed.
As the code just wants to update LOG_LINKS and LOG_LINKS are only for
registers, not for MEM or anything else, the patch just doesn't update those
if it isn't a REG or SUBREG of REG.
Bootstrapped/regtested on armv7hl-linux-gnueabi and powerpc64le-linux, ok
for trunk?
2020-03-31 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/94291
* combine.c (try_combine): For split_i2i3, don't assume SET_DEST
must be a REG or SUBREG of REG; if it is not one of these, don't
update LOG_LINKs.
* gcc.dg/pr94291.c: New test.
--- gcc/combine.c.jj 2020-02-25 13:56:19.514076091 +0100
+++ gcc/combine.c 2020-03-30 17:23:17.579654895 +0200
@@ -4351,25 +4351,29 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
if (GET_CODE (x) == PARALLEL)
x = XVECEXP (newi2pat, 0, 0);
- /* It can only be a SET of a REG or of a SUBREG of a REG. */
- unsigned int regno = reg_or_subregno (SET_DEST (x));
-
- bool done = false;
- for (rtx_insn *insn = NEXT_INSN (i3);
- !done
- && insn
- && NONDEBUG_INSN_P (insn)
- && BLOCK_FOR_INSN (insn) == this_basic_block;
- insn = NEXT_INSN (insn))
+ if (REG_P (SET_DEST (x))
+ || (GET_CODE (SET_DEST (x)) == SUBREG
+ && REG_P (SUBREG_REG (SET_DEST (x)))))
{
- struct insn_link *link;
- FOR_EACH_LOG_LINK (link, insn)
- if (link->insn == i3 && link->regno == regno)
- {
- link->insn = i2;
- done = true;
- break;
- }
+ unsigned int regno = reg_or_subregno (SET_DEST (x));
+
+ bool done = false;
+ for (rtx_insn *insn = NEXT_INSN (i3);
+ !done
+ && insn
+ && NONDEBUG_INSN_P (insn)
+ && BLOCK_FOR_INSN (insn) == this_basic_block;
+ insn = NEXT_INSN (insn))
+ {
+ struct insn_link *link;
+ FOR_EACH_LOG_LINK (link, insn)
+ if (link->insn == i3 && link->regno == regno)
+ {
+ link->insn = i2;
+ done = true;
+ break;
+ }
+ }
}
}
--- gcc/testsuite/gcc.dg/pr94291.c.jj 2020-03-30 17:22:47.876092906 +0200
+++ gcc/testsuite/gcc.dg/pr94291.c 2020-03-30 17:22:22.230471072 +0200
@@ -0,0 +1,14 @@
+/* PR rtl-optimization/94291 */
+/* { dg-do compile } */
+/* { dg-options "-Og" } */
+
+unsigned a;
+
+unsigned
+foo (void)
+{
+ unsigned x
+ = (__builtin_sub_overflow ((long long) a, 0, &x)
+ ? 1 : (__INTPTR_TYPE__) __builtin_memmove (&x, foo, 1));
+ return a;
+}
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Patch ping: Re: [PATCH] combine: Fix split_i2i3 ICE [PR94291]
2020-03-31 7:04 [PATCH] combine: Fix split_i2i3 ICE [PR94291] Jakub Jelinek
@ 2020-04-07 10:53 ` Jakub Jelinek
2020-04-07 19:18 ` Segher Boessenkool
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Jelinek @ 2020-04-07 10:53 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
Hi!
I'd like to ping this P1 PR fix:
On Tue, Mar 31, 2020 at 09:04:18AM +0200, Jakub Jelinek via Gcc-patches wrote:
> 2020-03-31 Jakub Jelinek <jakub@redhat.com>
>
> PR rtl-optimization/94291
> * combine.c (try_combine): For split_i2i3, don't assume SET_DEST
> must be a REG or SUBREG of REG; if it is not one of these, don't
> update LOG_LINKs.
>
> * gcc.dg/pr94291.c: New test.
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] combine: Fix split_i2i3 ICE [PR94291]
2020-03-31 7:04 [PATCH] combine: Fix split_i2i3 ICE [PR94291] Jakub Jelinek
2020-04-07 10:53 ` Patch ping: " Jakub Jelinek
@ 2020-04-07 19:18 ` Segher Boessenkool
1 sibling, 0 replies; 3+ messages in thread
From: Segher Boessenkool @ 2020-04-07 19:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
Hi!
(I have no idea why this PR is a P1).
On Tue, Mar 31, 2020 at 09:04:18AM +0200, Jakub Jelinek wrote:
> The following testcase ICEs on armv7hl-linux-gnueabi.
Also on default arm-linux-gnueabi. It needs -Og.
> try_combine is called on:
> (gdb) p debug_rtx (i3)
> (insn 20 12 22 2 (set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
> (const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
> (reg:SI 125)) "pr94291.c":7:8 241 {*arm_movsi_insn}
> (expr_list:REG_DEAD (reg:SI 125)
> (nil)))
> (gdb) p debug_rtx (i2)
> (insn 12 7 20 2 (parallel [
> (set (reg:CC 100 cc)
> (compare:CC (reg:SI 121 [ <retval> ])
> (const_int 0 [0])))
> (set (reg:SI 125)
> (reg:SI 121 [ <retval> ]))
> ]) "pr94291.c":7:8 248 {*movsi_compare0}
> (expr_list:REG_UNUSED (reg:CC 100 cc)
> (nil)))
> and tries to recognize cc = r121 cmp 0; [sfp-4] = r121 parallel,
> but that isn't recognized,
Trying 12 -> 20:
12: {cc:CC=cmp(r118:SI,0);r122:SI=r118:SI;}
REG_UNUSED cc:CC
20: [sfp:SI-0x4]=r122:SI
REG_DEAD r122:SI
Failed to match this instruction:
(parallel [
(set (reg:CC 100 cc)
(compare:CC (reg:SI 118 [ <retval> ])
(const_int 0 [0])))
(set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
(const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
(reg:SI 118 [ <retval> ]))
])
(twice)
> so it splits it into two: split_i2i3
(That variable does not *control* the splitting: it registers that we
did split a parallel into newi2pat and newpat).
Successfully matched this instruction:
(set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
(const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
(reg:SI 118 [ <retval> ]))
Successfully matched this instruction:
(set (reg:CC 100 cc)
(compare:CC (reg:SI 118 [ <retval> ])
(const_int 0 [0])))
allowing combination of insns 12 and 20
original costs 4 + 4 = 8
replacement costs 4 + 4 = 8
> [sfp-4] = r121 followed by cc = r121 cmp 0 which is recognized, but
> ICEs because the code below insist that the SET_DEST of newi2pat
> (or first set in PARALLEL thereof) must be a REG or SUBREG of REG,
> but it is a MEM in this case. I don't see any condition that would
> guarantee that, perhaps for the swap_i2i3 case it was somehow guaranteed.
I don't see how it was guaranteed for that case, either.
> As the code just wants to update LOG_LINKS and LOG_LINKS are only for
> registers, not for MEM or anything else, the patch just doesn't update those
> if it isn't a REG or SUBREG of REG.
That is correct.
> 2020-03-31 Jakub Jelinek <jakub@redhat.com>
>
> PR rtl-optimization/94291
Please mention PR rtl-optimization/84169 as well.
> * combine.c (try_combine): For split_i2i3, don't assume SET_DEST
> must be a REG or SUBREG of REG; if it is not one of these, don't
> update LOG_LINKs.
>
> * gcc.dg/pr94291.c: New test.
Okay for trunk, and for backports as well (after soaking it for a bit).
Thanks!
Segher
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-07 19:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31 7:04 [PATCH] combine: Fix split_i2i3 ICE [PR94291] Jakub Jelinek
2020-04-07 10:53 ` Patch ping: " Jakub Jelinek
2020-04-07 19:18 ` Segher Boessenkool
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).