* [PATCH] i386: Fix up handling of debug insns in STV [PR109676]
@ 2023-05-02 8:08 Jakub Jelinek
2023-05-04 3:17 ` Hongtao Liu
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2023-05-02 8:08 UTC (permalink / raw)
To: Uros Bizjak, Hongtao Liu, hjl.tools; +Cc: gcc-patches
Hi!
The following testcase ICEs because STV replaces there
(debug_insn 114 47 51 8 (var_location:TI D#3 (reg:TI 91 [ p ])) -1
(nil))
with
(debug_insn 114 47 51 8 (var_location:TI D#3 (reg:V1TI 91 [ p ])) -1
(nil))
which is invalid because of the mode mismatch.
STV has fix_debug_reg_uses function which is supposed to fix this up
and adjust such debug insns into
(debug_insn 114 47 51 8 (var_location:TI D#3 (subreg:TI (reg:V1TI 91 [ p ]) 0)) -1
(nil))
but it doesn't trigger here.
The IL before stv1 has:
(debug_insn 114 47 51 8 (var_location:TI D#3 (reg:TI 91 [ p ])) -1
(nil))
...
(insn 63 62 64 8 (set (mem/c:TI (reg/f:DI 89 [ .result_ptr ]) [0 <retval>.mStorage+0 S16 A32])
(reg:TI 91 [ p ])) "pr109676.C":4:48 87 {*movti_internal}
(expr_list:REG_DEAD (reg:TI 91 [ p ])
(nil)))
in bb 8 and
(insn 97 96 98 9 (set (reg:TI 91 [ p ])
(mem/c:TI (plus:DI (reg/f:DI 19 frame)
(const_int -32 [0xffffffffffffffe0])) [0 p+0 S16 A128])) "pr109676.C":26:12 87 {*movti_internal}
(nil))
(insn 98 97 99 9 (set (mem/c:TI (plus:DI (reg/f:DI 19 frame)
(const_int -64 [0xffffffffffffffc0])) [0 tmp+0 S16 A128])
(reg:TI 91 [ p ])) "pr109676.C":26:12 87 {*movti_internal}
(nil))
in bb9.
PUT_MODE on a REG is done in two spots in timode_scalar_chain::convert_insn,
one is:
switch (GET_CODE (dst))
{
case REG:
if (GET_MODE (dst) == TImode)
{
PUT_MODE (dst, V1TImode);
fix_debug_reg_uses (dst);
}
if (GET_MODE (dst) == V1TImode)
when seeing the REG in SET_DEST and another one the hunk the patch adjusts.
Because bb 8 comes first in the order the pass walks the bbs, we first
notice the TImode pseudo on insn 63 where it is SET_SRC, use PUT_MODE there
unconditionally, so for a shared REG it changes all other uses in the IL,
and then don't call fix_debug_reg_uses because DF_REG_DEF_CHAIN (REGNO (src))
is non-NULL - the REG is set in insn 97 but we haven't processed it yet.
Later on we process insn 97, but because the REG in SET_DEST already has
V1TImode, we don't do anything, even when the src handling code earlier
relied on it being done.
The following patch fixes this by using similar code for both dst and src,
in particular calling fix_debug_reg_uses once when we actually change REG
mode from TImode to V1TImode, and not later on.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/13.2?
2023-05-02 Jakub Jelinek <jakub@redhat.com>
PR debug/109676
* config/i386/i386-features.cc (timode_scalar_chain::convert_insn):
If src is REG, change its mode to V1TImode and call fix_debug_reg_uses
for it only if it still has TImode. Don't decide whether to call
fix_debug_reg_uses based on whether SRC is ever set or not.
* g++.target/i386/pr109676.C: New test.
--- gcc/config/i386/i386-features.cc.jj 2023-03-03 11:18:33.100296735 +0100
+++ gcc/config/i386/i386-features.cc 2023-05-01 14:50:45.559668773 +0200
@@ -1635,10 +1635,11 @@ timode_scalar_chain::convert_insn (rtx_i
switch (GET_CODE (src))
{
case REG:
- PUT_MODE (src, V1TImode);
- /* Call fix_debug_reg_uses only if SRC is never defined. */
- if (!DF_REG_DEF_CHAIN (REGNO (src)))
- fix_debug_reg_uses (src);
+ if (GET_MODE (src) == TImode)
+ {
+ PUT_MODE (src, V1TImode);
+ fix_debug_reg_uses (src);
+ }
break;
case MEM:
--- gcc/testsuite/g++.target/i386/pr109676.C.jj 2023-05-01 14:58:09.024329438 +0200
+++ gcc/testsuite/g++.target/i386/pr109676.C 2023-05-01 14:57:46.566650471 +0200
@@ -0,0 +1,46 @@
+// PR debug/109676
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2 -g -march=alderlake" }
+
+template <class T>
+struct A {
+ T a;
+ char b;
+ template <typename U>
+ A (U x, int) : a{x} {}
+ A (...);
+ T foo () { return a; }
+};
+bool bar ();
+struct B { int c, d; unsigned char e[8]; };
+bool baz ();
+struct C { C () : f () {} B &boo () { return f; } B f; };
+
+A<B>
+qux ()
+{
+ {
+ A<B> p;
+ bool t = true;
+ for (; bar ();)
+ if (baz ())
+ {
+ t = false;
+ break;
+ }
+ if (t)
+ p.b = false;
+ return p;
+ }
+}
+
+A<C>
+garply ()
+{
+ C g;
+ A<B> h = qux ();
+ if (!h.b)
+ return 0;
+ g.boo () = h.foo ();
+ return A<C>{g, 0};
+}
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] i386: Fix up handling of debug insns in STV [PR109676]
2023-05-02 8:08 [PATCH] i386: Fix up handling of debug insns in STV [PR109676] Jakub Jelinek
@ 2023-05-04 3:17 ` Hongtao Liu
0 siblings, 0 replies; 2+ messages in thread
From: Hongtao Liu @ 2023-05-04 3:17 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Uros Bizjak, Hongtao Liu, hjl.tools, gcc-patches
On Tue, May 2, 2023 at 4:08 PM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> The following testcase ICEs because STV replaces there
> (debug_insn 114 47 51 8 (var_location:TI D#3 (reg:TI 91 [ p ])) -1
> (nil))
> with
> (debug_insn 114 47 51 8 (var_location:TI D#3 (reg:V1TI 91 [ p ])) -1
> (nil))
> which is invalid because of the mode mismatch.
> STV has fix_debug_reg_uses function which is supposed to fix this up
> and adjust such debug insns into
> (debug_insn 114 47 51 8 (var_location:TI D#3 (subreg:TI (reg:V1TI 91 [ p ]) 0)) -1
> (nil))
> but it doesn't trigger here.
> The IL before stv1 has:
> (debug_insn 114 47 51 8 (var_location:TI D#3 (reg:TI 91 [ p ])) -1
> (nil))
> ...
> (insn 63 62 64 8 (set (mem/c:TI (reg/f:DI 89 [ .result_ptr ]) [0 <retval>.mStorage+0 S16 A32])
> (reg:TI 91 [ p ])) "pr109676.C":4:48 87 {*movti_internal}
> (expr_list:REG_DEAD (reg:TI 91 [ p ])
> (nil)))
> in bb 8 and
> (insn 97 96 98 9 (set (reg:TI 91 [ p ])
> (mem/c:TI (plus:DI (reg/f:DI 19 frame)
> (const_int -32 [0xffffffffffffffe0])) [0 p+0 S16 A128])) "pr109676.C":26:12 87 {*movti_internal}
> (nil))
> (insn 98 97 99 9 (set (mem/c:TI (plus:DI (reg/f:DI 19 frame)
> (const_int -64 [0xffffffffffffffc0])) [0 tmp+0 S16 A128])
> (reg:TI 91 [ p ])) "pr109676.C":26:12 87 {*movti_internal}
> (nil))
> in bb9.
> PUT_MODE on a REG is done in two spots in timode_scalar_chain::convert_insn,
> one is:
> switch (GET_CODE (dst))
> {
> case REG:
> if (GET_MODE (dst) == TImode)
> {
> PUT_MODE (dst, V1TImode);
> fix_debug_reg_uses (dst);
> }
> if (GET_MODE (dst) == V1TImode)
> when seeing the REG in SET_DEST and another one the hunk the patch adjusts.
> Because bb 8 comes first in the order the pass walks the bbs, we first
> notice the TImode pseudo on insn 63 where it is SET_SRC, use PUT_MODE there
> unconditionally, so for a shared REG it changes all other uses in the IL,
> and then don't call fix_debug_reg_uses because DF_REG_DEF_CHAIN (REGNO (src))
> is non-NULL - the REG is set in insn 97 but we haven't processed it yet.
> Later on we process insn 97, but because the REG in SET_DEST already has
> V1TImode, we don't do anything, even when the src handling code earlier
> relied on it being done.
>
> The following patch fixes this by using similar code for both dst and src,
> in particular calling fix_debug_reg_uses once when we actually change REG
> mode from TImode to V1TImode, and not later on.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/13.2?
Thanks for the clear explanation, the patch LGTM.
>
> 2023-05-02 Jakub Jelinek <jakub@redhat.com>
>
> PR debug/109676
> * config/i386/i386-features.cc (timode_scalar_chain::convert_insn):
> If src is REG, change its mode to V1TImode and call fix_debug_reg_uses
> for it only if it still has TImode. Don't decide whether to call
> fix_debug_reg_uses based on whether SRC is ever set or not.
>
> * g++.target/i386/pr109676.C: New test.
>
> --- gcc/config/i386/i386-features.cc.jj 2023-03-03 11:18:33.100296735 +0100
> +++ gcc/config/i386/i386-features.cc 2023-05-01 14:50:45.559668773 +0200
> @@ -1635,10 +1635,11 @@ timode_scalar_chain::convert_insn (rtx_i
> switch (GET_CODE (src))
> {
> case REG:
> - PUT_MODE (src, V1TImode);
> - /* Call fix_debug_reg_uses only if SRC is never defined. */
> - if (!DF_REG_DEF_CHAIN (REGNO (src)))
> - fix_debug_reg_uses (src);
> + if (GET_MODE (src) == TImode)
> + {
> + PUT_MODE (src, V1TImode);
> + fix_debug_reg_uses (src);
> + }
> break;
>
> case MEM:
> --- gcc/testsuite/g++.target/i386/pr109676.C.jj 2023-05-01 14:58:09.024329438 +0200
> +++ gcc/testsuite/g++.target/i386/pr109676.C 2023-05-01 14:57:46.566650471 +0200
> @@ -0,0 +1,46 @@
> +// PR debug/109676
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -g -march=alderlake" }
> +
> +template <class T>
> +struct A {
> + T a;
> + char b;
> + template <typename U>
> + A (U x, int) : a{x} {}
> + A (...);
> + T foo () { return a; }
> +};
> +bool bar ();
> +struct B { int c, d; unsigned char e[8]; };
> +bool baz ();
> +struct C { C () : f () {} B &boo () { return f; } B f; };
> +
> +A<B>
> +qux ()
> +{
> + {
> + A<B> p;
> + bool t = true;
> + for (; bar ();)
> + if (baz ())
> + {
> + t = false;
> + break;
> + }
> + if (t)
> + p.b = false;
> + return p;
> + }
> +}
> +
> +A<C>
> +garply ()
> +{
> + C g;
> + A<B> h = qux ();
> + if (!h.b)
> + return 0;
> + g.boo () = h.foo ();
> + return A<C>{g, 0};
> +}
>
> Jakub
>
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-05-04 3:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-02 8:08 [PATCH] i386: Fix up handling of debug insns in STV [PR109676] Jakub Jelinek
2023-05-04 3:17 ` Hongtao Liu
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).