* [PATCH] recog: Return 1 from insn_invalid_p if REG_INC reg overlaps some stored reg [PR103775]
@ 2022-03-25 10:17 Jakub Jelinek
2022-03-25 17:24 ` Jeff Law
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2022-03-25 10:17 UTC (permalink / raw)
To: Richard Biener, Eric Botcazou, Jeff Law; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3805 bytes --]
Hi!
The following testcase ICEs on aarch64-linux with -g and
assembles with a warning otherwise, because it emits
ldrb w0,[x0,16]!
instruction which sets the x0 register multiple times.
Due to disabled DCE (from -Og) we end up before REE with:
(insn 12 39 13 2 (set (reg:SI 1 x1 [orig:93 _2 ] [93])
(zero_extend:SI (mem/c:QI (pre_modify:DI (reg/f:DI 0 x0 [114])
(plus:DI (reg/f:DI 0 x0 [114])
(const_int 16 [0x10]))) [1 u128_1+0 S1 A128]))) "pr103775.c":5:35 117 {*zero_extendqisi2_aarch64}
(expr_list:REG_INC (reg/f:DI 0 x0 [114])
(nil)))
(insn 13 12 14 2 (set (reg:DI 0 x0 [orig:112 _2 ] [112])
(zero_extend:DI (reg:SI 1 x1 [orig:93 _2 ] [93]))) "pr103775.c":5:16 111 {*zero_extendsidi2_aarch64}
(nil))
which is valid but not exactly efficient as x0 is dead after the
insn that auto-increments it. REE turns it into:
(insn 12 39 44 2 (set (reg:DI 0 x0)
(zero_extend:DI (mem/c:QI (pre_modify:DI (reg/f:DI 0 x0 [114])
(plus:DI (reg/f:DI 0 x0 [114])
(const_int 16 [0x10]))) [1 u128_1+0 S1 A128]))) "pr103775.c":5:35 119 {*zero_extendqidi2_aarch64}
(expr_list:REG_INC (reg/f:DI 0 x0 [114])
(nil)))
(insn 44 12 14 2 (set (reg:DI 1 x1)
(reg:DI 0 x0)) "pr103775.c":5:35 -1
(nil))
which is invalid because it sets x0 multiple times, one
in SET_DEST of the PATTERN and once in PRE_MODIFY.
As perhaps other passes than REE might suffer from it, IMHO it is better
to reject this during change validation.
Below is one patch that does that only if reload_completed, attached
is another version that does it always even before reload.
I've so far bootstrapped/regtested the first patch on
{x86_64,i686,powerpc64le,armv7hl}-linux, aarch64-linux regtest
is still pending.
If you prefer the second version, I can start testing it momentarily.
2022-03-25 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/103775
* recog.cc (check_invalid_inc_dec): New function.
(insn_invalid_p): Return 1 if REG_INC operand overlaps
any stored REGs.
* gcc.dg/pr103775.c: New test.
--- gcc/recog.cc.jj 2022-01-18 11:58:59.802978913 +0100
+++ gcc/recog.cc 2022-03-24 17:46:00.116489292 +0100
@@ -329,6 +329,17 @@ canonicalize_change_group (rtx_insn *ins
return false;
}
+/* Check if REG_INC argument in *data overlaps a stored REG. */
+
+static void
+check_invalid_inc_dec (rtx reg, const_rtx, void *data)
+{
+ rtx *pinc = (rtx *) data;
+ if (*pinc == NULL_RTX || MEM_P (reg))
+ return;
+ if (reg_overlap_mentioned_p (reg, *pinc))
+ *pinc = NULL_RTX;
+}
/* This subroutine of apply_change_group verifies whether the changes to INSN
were valid; i.e. whether INSN can still be recognized.
@@ -384,6 +395,17 @@ insn_invalid_p (rtx_insn *insn, bool in_
if (! constrain_operands (1, get_preferred_alternatives (insn)))
return 1;
+
+ /* Punt if REG_INC argument overlaps some stored REG. */
+ for (rtx link = FIND_REG_INC_NOTE (insn, NULL_RTX);
+ link; link = XEXP (link, 1))
+ if (REG_NOTE_KIND (link) == REG_INC)
+ {
+ rtx reg = XEXP (link, 0);
+ note_stores (insn, check_invalid_inc_dec, ®);
+ if (reg == NULL_RTX)
+ return 1;
+ }
}
INSN_CODE (insn) = icode;
--- gcc/testsuite/gcc.dg/pr103775.c.jj 2022-03-24 17:51:25.962817859 +0100
+++ gcc/testsuite/gcc.dg/pr103775.c 2022-03-24 17:51:11.024032506 +0100
@@ -0,0 +1,12 @@
+/* PR rtl-optimization/103775 */
+/* { dg-do assemble { target int128 } } */
+/* { dg-options "-Og -fno-forward-propagate -free -g" } */
+
+int
+foo (char a, short b, int c, __int128 d, char e, short f, int g, __int128 h)
+{
+ long i = __builtin_clrsbll ((char) h);
+ __builtin_memset ((char *) &h + 4, d, 3);
+ c &= (char) h;
+ return c + i;
+}
Jakub
[-- Attachment #2: R412a --]
[-- Type: text/plain, Size: 1837 bytes --]
2022-03-25 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/103775
* recog.cc (check_invalid_inc_dec): New function.
(insn_invalid_p): Return 1 if REG_INC operand overlaps
any stored REGs.
* gcc.dg/pr103775.c: New test.
--- gcc/recog.cc.jj 2022-01-18 11:58:59.802978913 +0100
+++ gcc/recog.cc 2022-03-24 17:46:00.116489292 +0100
@@ -329,6 +329,17 @@ canonicalize_change_group (rtx_insn *ins
return false;
}
+/* Check if REG_INC argument in *data overlaps a stored REG. */
+
+static void
+check_invalid_inc_dec (rtx reg, const_rtx, void *data)
+{
+ rtx *pinc = (rtx *) data;
+ if (*pinc == NULL_RTX || MEM_P (reg))
+ return;
+ if (reg_overlap_mentioned_p (reg, *pinc))
+ *pinc = NULL_RTX;
+}
/* This subroutine of apply_change_group verifies whether the changes to INSN
were valid; i.e. whether INSN can still be recognized.
@@ -386,6 +397,17 @@ insn_invalid_p (rtx_insn *insn, bool in_
return 1;
}
+ /* Punt if REG_INC argument overlaps some stored REG. */
+ for (rtx link = FIND_REG_INC_NOTE (insn, NULL_RTX);
+ link; link = XEXP (link, 1))
+ if (REG_NOTE_KIND (link) == REG_INC)
+ {
+ rtx reg = XEXP (link, 0);
+ note_stores (insn, check_invalid_inc_dec, ®);
+ if (reg == NULL_RTX)
+ return 1;
+ }
+
INSN_CODE (insn) = icode;
return 0;
}
--- gcc/testsuite/gcc.dg/pr103775.c.jj 2022-03-24 17:51:25.962817859 +0100
+++ gcc/testsuite/gcc.dg/pr103775.c 2022-03-24 17:51:11.024032506 +0100
@@ -0,0 +1,12 @@
+/* PR rtl-optimization/103775 */
+/* { dg-do assemble { target int128 } } */
+/* { dg-options "-Og -fno-forward-propagate -free -g" } */
+
+int
+foo (char a, short b, int c, __int128 d, char e, short f, int g, __int128 h)
+{
+ long i = __builtin_clrsbll ((char) h);
+ __builtin_memset ((char *) &h + 4, d, 3);
+ c &= (char) h;
+ return c + i;
+}
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] recog: Return 1 from insn_invalid_p if REG_INC reg overlaps some stored reg [PR103775]
2022-03-25 10:17 [PATCH] recog: Return 1 from insn_invalid_p if REG_INC reg overlaps some stored reg [PR103775] Jakub Jelinek
@ 2022-03-25 17:24 ` Jeff Law
0 siblings, 0 replies; 2+ messages in thread
From: Jeff Law @ 2022-03-25 17:24 UTC (permalink / raw)
To: Jakub Jelinek, Richard Biener, Eric Botcazou; +Cc: gcc-patches
On 3/25/2022 4:17 AM, Jakub Jelinek wrote:
> Hi!
>
> The following testcase ICEs on aarch64-linux with -g and
> assembles with a warning otherwise, because it emits
> ldrb w0,[x0,16]!
> instruction which sets the x0 register multiple times.
> Due to disabled DCE (from -Og) we end up before REE with:
> (insn 12 39 13 2 (set (reg:SI 1 x1 [orig:93 _2 ] [93])
> (zero_extend:SI (mem/c:QI (pre_modify:DI (reg/f:DI 0 x0 [114])
> (plus:DI (reg/f:DI 0 x0 [114])
> (const_int 16 [0x10]))) [1 u128_1+0 S1 A128]))) "pr103775.c":5:35 117 {*zero_extendqisi2_aarch64}
> (expr_list:REG_INC (reg/f:DI 0 x0 [114])
> (nil)))
> (insn 13 12 14 2 (set (reg:DI 0 x0 [orig:112 _2 ] [112])
> (zero_extend:DI (reg:SI 1 x1 [orig:93 _2 ] [93]))) "pr103775.c":5:16 111 {*zero_extendsidi2_aarch64}
> (nil))
> which is valid but not exactly efficient as x0 is dead after the
> insn that auto-increments it. REE turns it into:
> (insn 12 39 44 2 (set (reg:DI 0 x0)
> (zero_extend:DI (mem/c:QI (pre_modify:DI (reg/f:DI 0 x0 [114])
> (plus:DI (reg/f:DI 0 x0 [114])
> (const_int 16 [0x10]))) [1 u128_1+0 S1 A128]))) "pr103775.c":5:35 119 {*zero_extendqidi2_aarch64}
> (expr_list:REG_INC (reg/f:DI 0 x0 [114])
> (nil)))
> (insn 44 12 14 2 (set (reg:DI 1 x1)
> (reg:DI 0 x0)) "pr103775.c":5:35 -1
> (nil))
> which is invalid because it sets x0 multiple times, one
> in SET_DEST of the PATTERN and once in PRE_MODIFY.
> As perhaps other passes than REE might suffer from it, IMHO it is better
> to reject this during change validation.
>
> Below is one patch that does that only if reload_completed, attached
> is another version that does it always even before reload.
>
> I've so far bootstrapped/regtested the first patch on
> {x86_64,i686,powerpc64le,armv7hl}-linux, aarch64-linux regtest
> is still pending.
> If you prefer the second version, I can start testing it momentarily.
>
> 2022-03-25 Jakub Jelinek<jakub@redhat.com>
>
> PR rtl-optimization/103775
> * recog.cc (check_invalid_inc_dec): New function.
> (insn_invalid_p): Return 1 if REG_INC operand overlaps
> any stored REGs.
>
> * gcc.dg/pr103775.c: New test.
OK. I bet this could also be extended to catch the case where the
autoinc'd operand is used as a source elsewhere in the insn. That's
supposed to be avoided in RTL, but we don't check it and I've seen it
sneak in (see PR 101697):
> (insn 1444 1443 164 31 (parallel [
> (set (mem/f:SI (pre_dec:SI (reg/f:SI 7 sp)) [3 S4 A32])
> (reg/f:SI 7 sp))
> (clobber (reg:CC 12 cc))
> ]) "libc/inet/getaddrinfo.c":466:11 -1
> (expr_list:REG_ARGS_SIZE (const_int 4 [0x4])
> (nil)))
While this has well defined semantics on the H8 RTL considers it
ill-defined, but never checks for it.
> If a register used as the operand of these expressions is used in
> another address in an insn, the original value of the register is used.
> Uses of the register outside of an address are not permitted within the
> same insn as a use in an embedded side effect expression because such
> insns behave differently on different machines and hence must be treated
> as ambiguous and disallowed.
Jeff
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-25 17:24 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-25 10:17 [PATCH] recog: Return 1 from insn_invalid_p if REG_INC reg overlaps some stored reg [PR103775] Jakub Jelinek
2022-03-25 17:24 ` Jeff Law
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).