[PR106746] drop cselib addr lookup in debug insn mem

Message ID orcz7hxsqf.fsf@lxoliva.fsfla.org
State Accepted
Headers
Series [PR106746] drop cselib addr lookup in debug insn mem |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Alexandre Oliva Jan. 14, 2023, 11:26 a.m. UTC
  The testcase used to get scheduled differently depending on the
presence of debug insns with MEMs.  It's not clear to me why those
MEMs affected scheduling, but the cselib pre-canonicalization of the
MEM address is not used at all when analyzing debug insns, so the
memory allocation and lookup are pure waste.  Somehow, avoiding that
waste fixes the problem, or makes it go latent.

Regstrapped on x86_64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	PR debug/106746
	* sched-deps.cc (sched_analyze_2): Skip cselib address lookup
	within debug insns.

for  gcc/testsuite/ChangeLog

	PR debug/106746
	* gcc.dg/target/i386/pr106746.c: New.
---
 gcc/sched-deps.cc                        |   36 +++++++++++++++---------------
 gcc/testsuite/gcc.target/i386/pr106746.c |   29 ++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr106746.c
  

Comments

Richard Biener Jan. 16, 2023, 7:29 a.m. UTC | #1
On Sat, Jan 14, 2023 at 12:26 PM Alexandre Oliva via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> The testcase used to get scheduled differently depending on the
> presence of debug insns with MEMs.  It's not clear to me why those
> MEMs affected scheduling, but the cselib pre-canonicalization of the
> MEM address is not used at all when analyzing debug insns, so the
> memory allocation and lookup are pure waste.  Somehow, avoiding that
> waste fixes the problem, or makes it go latent.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?

OK.

Richard.

>
> for  gcc/ChangeLog
>
>         PR debug/106746
>         * sched-deps.cc (sched_analyze_2): Skip cselib address lookup
>         within debug insns.
>
> for  gcc/testsuite/ChangeLog
>
>         PR debug/106746
>         * gcc.dg/target/i386/pr106746.c: New.
> ---
>  gcc/sched-deps.cc                        |   36 +++++++++++++++---------------
>  gcc/testsuite/gcc.target/i386/pr106746.c |   29 ++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 18 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr106746.c
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index f9371b81fb41e..a9214f674329a 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -2605,26 +2605,26 @@ sched_analyze_2 (class deps_desc *deps, rtx x, rtx_insn *insn)
>
>      case MEM:
>        {
> -       /* Reading memory.  */
> -       rtx_insn_list *u;
> -       rtx_insn_list *pending;
> -       rtx_expr_list *pending_mem;
> -       rtx t = x;
> -
> -       if (sched_deps_info->use_cselib)
> -         {
> -           machine_mode address_mode = get_address_mode (t);
> -
> -           t = shallow_copy_rtx (t);
> -           cselib_lookup_from_insn (XEXP (t, 0), address_mode, 1,
> -                                    GET_MODE (t), insn);
> -           XEXP (t, 0)
> -             = cselib_subst_to_values_from_insn (XEXP (t, 0), GET_MODE (t),
> -                                                 insn);
> -         }
> -
>         if (!DEBUG_INSN_P (insn))
>           {
> +           /* Reading memory.  */
> +           rtx_insn_list *u;
> +           rtx_insn_list *pending;
> +           rtx_expr_list *pending_mem;
> +           rtx t = x;
> +
> +           if (sched_deps_info->use_cselib)
> +             {
> +               machine_mode address_mode = get_address_mode (t);
> +
> +               t = shallow_copy_rtx (t);
> +               cselib_lookup_from_insn (XEXP (t, 0), address_mode, 1,
> +                                        GET_MODE (t), insn);
> +               XEXP (t, 0)
> +                 = cselib_subst_to_values_from_insn (XEXP (t, 0), GET_MODE (t),
> +                                                     insn);
> +             }
> +
>             t = canon_rtx (t);
>             pending = deps->pending_read_insns;
>             pending_mem = deps->pending_read_mems;
> diff --git a/gcc/testsuite/gcc.target/i386/pr106746.c b/gcc/testsuite/gcc.target/i386/pr106746.c
> new file mode 100644
> index 0000000000000..14f7dab71d691
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr106746.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fsched2-use-superblocks -fcompare-debug -Wno-psabi" } */
> +
> +typedef char __attribute__((__vector_size__ (64))) U;
> +typedef short __attribute__((__vector_size__ (64))) V;
> +typedef int __attribute__((__vector_size__ (64))) W;
> +
> +char c;
> +U a;
> +U *r;
> +W foo0_v512u32_0;
> +
> +void
> +foo (W)
> +{
> +  U u;
> +  V v;
> +  W w = __builtin_shuffle (foo0_v512u32_0, foo0_v512u32_0);
> +  u =
> +    __builtin_shufflevector (a, u, 3, 0, 4, 9, 9, 6, 7, 8, 5,
> +                            0, 6, 1, 8, 1, 2, 8, 6,
> +                            1, 8, 4, 9, 3, 8, 4, 6, 0, 9, 0, 1, 8, 2, 3, 3,
> +                            0, 4, 9, 9, 6, 7, 8, 5,
> +                            0, 6, 1, 8, 1, 2, 8, 6,
> +                            1, 8, 4, 9, 3, 8, 4, 6, 0, 9, 0, 1, 8, 2, 3);
> +  v *= c;
> +  w &= c;
> +  *r = (U) v + (U) w;
> +}
>
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>
  

Patch

diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index f9371b81fb41e..a9214f674329a 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -2605,26 +2605,26 @@  sched_analyze_2 (class deps_desc *deps, rtx x, rtx_insn *insn)
 
     case MEM:
       {
-	/* Reading memory.  */
-	rtx_insn_list *u;
-	rtx_insn_list *pending;
-	rtx_expr_list *pending_mem;
-	rtx t = x;
-
-	if (sched_deps_info->use_cselib)
-	  {
-	    machine_mode address_mode = get_address_mode (t);
-
-	    t = shallow_copy_rtx (t);
-	    cselib_lookup_from_insn (XEXP (t, 0), address_mode, 1,
-				     GET_MODE (t), insn);
-	    XEXP (t, 0)
-	      = cselib_subst_to_values_from_insn (XEXP (t, 0), GET_MODE (t),
-						  insn);
-	  }
-
 	if (!DEBUG_INSN_P (insn))
 	  {
+	    /* Reading memory.  */
+	    rtx_insn_list *u;
+	    rtx_insn_list *pending;
+	    rtx_expr_list *pending_mem;
+	    rtx t = x;
+
+	    if (sched_deps_info->use_cselib)
+	      {
+		machine_mode address_mode = get_address_mode (t);
+
+		t = shallow_copy_rtx (t);
+		cselib_lookup_from_insn (XEXP (t, 0), address_mode, 1,
+					 GET_MODE (t), insn);
+		XEXP (t, 0)
+		  = cselib_subst_to_values_from_insn (XEXP (t, 0), GET_MODE (t),
+						      insn);
+	      }
+
 	    t = canon_rtx (t);
 	    pending = deps->pending_read_insns;
 	    pending_mem = deps->pending_read_mems;
diff --git a/gcc/testsuite/gcc.target/i386/pr106746.c b/gcc/testsuite/gcc.target/i386/pr106746.c
new file mode 100644
index 0000000000000..14f7dab71d691
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr106746.c
@@ -0,0 +1,29 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fsched2-use-superblocks -fcompare-debug -Wno-psabi" } */
+
+typedef char __attribute__((__vector_size__ (64))) U;
+typedef short __attribute__((__vector_size__ (64))) V;
+typedef int __attribute__((__vector_size__ (64))) W;
+
+char c;
+U a;
+U *r;
+W foo0_v512u32_0;
+
+void
+foo (W)
+{
+  U u;
+  V v;
+  W w = __builtin_shuffle (foo0_v512u32_0, foo0_v512u32_0);
+  u =
+    __builtin_shufflevector (a, u, 3, 0, 4, 9, 9, 6, 7, 8, 5,
+			     0, 6, 1, 8, 1, 2, 8, 6,
+			     1, 8, 4, 9, 3, 8, 4, 6, 0, 9, 0, 1, 8, 2, 3, 3,
+			     0, 4, 9, 9, 6, 7, 8, 5,
+			     0, 6, 1, 8, 1, 2, 8, 6,
+			     1, 8, 4, 9, 3, 8, 4, 6, 0, 9, 0, 1, 8, 2, 3);
+  v *= c;
+  w &= c;
+  *r = (U) v + (U) w;
+}