* [PATCH] expand: Fix asm goto expansion [PR113415]
@ 2024-02-09 9:28 Jakub Jelinek
2024-02-09 9:34 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2024-02-09 9:28 UTC (permalink / raw)
To: Richard Biener, Jeff Law, Vladimir Makarov; +Cc: gcc-patches
Hi!
The asm goto expansion ICEs on the following testcase (which normally
is rejected later), because expand_asm_stmt emits the code to copy
the large var out of the out operand to its memory location into
after_rtl_seq ... after_rtl_end sequence and because it is asm goto,
it duplicates the sequence on each successor edge of the asm goto.
The problem is that with -mstringop-strategy=byte_loop that sequence
contains loops, so CODE_LABELs, JUMP_INSNs, with other strategies
could contain CALL_INSNs etc.
But the copying is done using a loop doing
emit_insn (copy_insn (PATTERN (curr)));
which does the right thing solely for INSNs, it will do the wrong thing
for JUMP_INSNs, CALL_INSNs, CODE_LABELs (with RTL checking even ICE on
them), BARRIERs and the like.
The following patch partially fixes it (with the hope that such stuff only
occurs in asms that really can't be accepted; if one uses say "=rm" or
"=g" constraint then the operand uses the memory directly and nothing is
copied) by using the
duplicate_insn_chain function which is used e.g. in RTL loop unrolling and
which can handle JUMP_INSNs, CALL_INSNs, BARRIERs etc.
As it is meant to operate on sequences inside of basic blocks, it doesn't
handle CODE_LABELs (well, it skips them), so if we need a solution that
will be correct at runtime here for those cases, we'd need to do further
work (e.g. still use duplicate_insn_chain, but if we notice any CODE_LABELs,
walk the sequence again, add copies of the CODE_LABELs and then remap
references to the old CODE_LABELs in the copied sequence to the new ones).
Because as is now, if the code in one of the sequence copies (where the
CODE_LABELs have been left out) decides to jump to such a CODE_LABEL, it
will jump to the CODE_LABEL which has been in the original sequence (which
the code emits on the last edge, after all, duplicating the sequence
EDGE_COUNT times and throwing away the original was wasteful, compared to
doing that just EDGE_COUNT - 1 times and using the original.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Or do need to handle even CODE_LABELs?
2024-02-09 Jakub Jelinek <jakub@redhat.com>
PR middle-end/113415
* cfgexpand.cc (expand_asm_stmt): For asm goto, use
duplicate_insn_chain to duplicate after_rtl_seq sequence instead
of hand written loop with emit_insn of copy_insn and emit original
after_rtl_seq on the last edge.
* gcc.target/i386/pr113415.c: New test.
--- gcc/cfgexpand.cc.jj 2024-01-24 13:11:21.011469855 +0100
+++ gcc/cfgexpand.cc 2024-02-08 18:22:04.699621085 +0100
@@ -3671,16 +3671,21 @@ expand_asm_stmt (gasm *stmt)
{
edge e;
edge_iterator ei;
-
+ unsigned int cnt = EDGE_COUNT (gimple_bb (stmt)->succs);
+
FOR_EACH_EDGE (e, ei, gimple_bb (stmt)->succs)
{
- start_sequence ();
- for (rtx_insn *curr = after_rtl_seq;
- curr != NULL_RTX;
- curr = NEXT_INSN (curr))
- emit_insn (copy_insn (PATTERN (curr)));
- rtx_insn *copy = get_insns ();
- end_sequence ();
+ rtx_insn *copy;
+ if (--cnt == 0)
+ copy = after_rtl_seq;
+ else
+ {
+ start_sequence ();
+ duplicate_insn_chain (after_rtl_seq, after_rtl_end,
+ NULL, NULL);
+ copy = get_insns ();
+ end_sequence ();
+ }
insert_insn_on_edge (copy, e);
}
}
--- gcc/testsuite/gcc.target/i386/pr113415.c.jj 2024-02-08 18:26:27.622966847 +0100
+++ gcc/testsuite/gcc.target/i386/pr113415.c 2024-02-08 18:26:11.336193222 +0100
@@ -0,0 +1,11 @@
+/* PR middle-end/113415 */
+/* { dg-do compile } */
+/* { dg-options "-mstringop-strategy=byte_loop" } */
+
+void
+foo (void)
+{
+ unsigned long arr[64];
+lab:
+ __asm__ goto ("" : "=r" (arr) : : : lab); /* { dg-error "impossible constraint in 'asm'" } */
+}
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] expand: Fix asm goto expansion [PR113415]
2024-02-09 9:28 [PATCH] expand: Fix asm goto expansion [PR113415] Jakub Jelinek
@ 2024-02-09 9:34 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2024-02-09 9:34 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jeff Law, Vladimir Makarov, gcc-patches
On Fri, 9 Feb 2024, Jakub Jelinek wrote:
> Hi!
>
> The asm goto expansion ICEs on the following testcase (which normally
> is rejected later), because expand_asm_stmt emits the code to copy
> the large var out of the out operand to its memory location into
> after_rtl_seq ... after_rtl_end sequence and because it is asm goto,
> it duplicates the sequence on each successor edge of the asm goto.
> The problem is that with -mstringop-strategy=byte_loop that sequence
> contains loops, so CODE_LABELs, JUMP_INSNs, with other strategies
> could contain CALL_INSNs etc.
> But the copying is done using a loop doing
> emit_insn (copy_insn (PATTERN (curr)));
> which does the right thing solely for INSNs, it will do the wrong thing
> for JUMP_INSNs, CALL_INSNs, CODE_LABELs (with RTL checking even ICE on
> them), BARRIERs and the like.
>
> The following patch partially fixes it (with the hope that such stuff only
> occurs in asms that really can't be accepted; if one uses say "=rm" or
> "=g" constraint then the operand uses the memory directly and nothing is
> copied) by using the
> duplicate_insn_chain function which is used e.g. in RTL loop unrolling and
> which can handle JUMP_INSNs, CALL_INSNs, BARRIERs etc.
> As it is meant to operate on sequences inside of basic blocks, it doesn't
> handle CODE_LABELs (well, it skips them), so if we need a solution that
> will be correct at runtime here for those cases, we'd need to do further
> work (e.g. still use duplicate_insn_chain, but if we notice any CODE_LABELs,
> walk the sequence again, add copies of the CODE_LABELs and then remap
> references to the old CODE_LABELs in the copied sequence to the new ones).
> Because as is now, if the code in one of the sequence copies (where the
> CODE_LABELs have been left out) decides to jump to such a CODE_LABEL, it
> will jump to the CODE_LABEL which has been in the original sequence (which
> the code emits on the last edge, after all, duplicating the sequence
> EDGE_COUNT times and throwing away the original was wasteful, compared to
> doing that just EDGE_COUNT - 1 times and using the original.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
> Or do need to handle even CODE_LABELs?
I guess we'll do when we run into it? I'm not sure we can constrain
asm gotos in a way such complicated "after" sequences never happen,
right? A sorry () might be a possibility for cases we don't handle.
Richard.
> 2024-02-09 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/113415
> * cfgexpand.cc (expand_asm_stmt): For asm goto, use
> duplicate_insn_chain to duplicate after_rtl_seq sequence instead
> of hand written loop with emit_insn of copy_insn and emit original
> after_rtl_seq on the last edge.
>
> * gcc.target/i386/pr113415.c: New test.
>
> --- gcc/cfgexpand.cc.jj 2024-01-24 13:11:21.011469855 +0100
> +++ gcc/cfgexpand.cc 2024-02-08 18:22:04.699621085 +0100
> @@ -3671,16 +3671,21 @@ expand_asm_stmt (gasm *stmt)
> {
> edge e;
> edge_iterator ei;
> -
> + unsigned int cnt = EDGE_COUNT (gimple_bb (stmt)->succs);
> +
> FOR_EACH_EDGE (e, ei, gimple_bb (stmt)->succs)
> {
> - start_sequence ();
> - for (rtx_insn *curr = after_rtl_seq;
> - curr != NULL_RTX;
> - curr = NEXT_INSN (curr))
> - emit_insn (copy_insn (PATTERN (curr)));
> - rtx_insn *copy = get_insns ();
> - end_sequence ();
> + rtx_insn *copy;
> + if (--cnt == 0)
> + copy = after_rtl_seq;
> + else
> + {
> + start_sequence ();
> + duplicate_insn_chain (after_rtl_seq, after_rtl_end,
> + NULL, NULL);
> + copy = get_insns ();
> + end_sequence ();
> + }
> insert_insn_on_edge (copy, e);
> }
> }
> --- gcc/testsuite/gcc.target/i386/pr113415.c.jj 2024-02-08 18:26:27.622966847 +0100
> +++ gcc/testsuite/gcc.target/i386/pr113415.c 2024-02-08 18:26:11.336193222 +0100
> @@ -0,0 +1,11 @@
> +/* PR middle-end/113415 */
> +/* { dg-do compile } */
> +/* { dg-options "-mstringop-strategy=byte_loop" } */
> +
> +void
> +foo (void)
> +{
> + unsigned long arr[64];
> +lab:
> + __asm__ goto ("" : "=r" (arr) : : : lab); /* { dg-error "impossible constraint in 'asm'" } */
> +}
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-02-09 9:34 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-09 9:28 [PATCH] expand: Fix asm goto expansion [PR113415] Jakub Jelinek
2024-02-09 9:34 ` Richard Biener
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).