Re: [PATCH] i386: Fix up handling of debug insns in STV [PR109676]

[Hongtao Liu <crazylht@gmail.com>]

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