Re: [PATCH 2/3]middle-end match.pd: simplify debug dump checks

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Apr 18, 2023 at 12:22 PM Tamar Christina via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi All,
>
> This is a small improvement in QoL codegen for match.pd to save time not
> re-evaluating the condition for printing debug information in every function.
>
> There is a small but consistent runtime and compile time win here.  The runtime
> win comes from not having to do the condition over again, and on Arm plaforms
> we now use the new test-and-branch support for booleans to only have a single
> instruction here.
>
> Compile time win is gotten from not having to do all the string parsing for the
> printf and having less string interning to do.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?

Ugh, I don't like the new global very much.  Can't we compute it in the
toplevel entry and pass it down as parameter?  Like passing down the
actual dump FILE *?

The file output in output_line_directive was because we originally had
match.pd #includeing multiple match-*.pd files, we'd want to keep that
supported I think.  But since the line directives are commented and
there's the same info available below, like

/* #line 798 "/home/rguenther/src/gcc-13-branch/gcc/match.pd" */
                          tree captures[2] ATTRIBUTE_UNUSED = { _p0, _p1 };
                          if (UNLIKELY (dump_file && (dump_flags &
TDF_FOLDING))) fprintf (dump_file, "Matching expression %s:%d,
%s:%d\n", "match.pd", 798, __FILE__, __LINE__);

there's probably no point in emitting them anymore (originally I emitted
them non-commented but that didn't improve debugging much).  We might
want to emit more "proper" line directives for the natively copied parts
of match.pd when code-generating c_expr parts, but that would be something
separate.

Can you split the patch into two things?  A patch removing output of
the commented line directives at the call sites is OK.

Richard.

> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
>         PR bootstrap/84402
>         * dumpfile.h (dump_folding_p): New.
>         * dumpfile.cc (set_dump_file): Use it.
>         * generic-match-head.cc (dump_debug): New.
>         * gimple-match-head.cc (dump_debug): New.
>         * genmatch.cc (output_line_directive):  Support outputting only line
>         because file is implied.
>         (dt_simplify::gen_1): Call debug_dump instead of printf.
>
> --- inline copy of patch --
> diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
> index 7d5eca899dcc98676a9ce7a7efff8e439854ff89..e7b595ddecdcca9983d9584b8b2417ae1941c7d4 100644
> --- a/gcc/dumpfile.h
> +++ b/gcc/dumpfile.h
> @@ -522,6 +522,7 @@ parse_dump_option (const char *, const char **);
>  extern FILE *dump_file;
>  extern dump_flags_t dump_flags;
>  extern const char *dump_file_name;
> +extern bool dump_folding_p;
>
>  extern bool dumps_are_enabled;
>
> diff --git a/gcc/dumpfile.cc b/gcc/dumpfile.cc
> index 51f68c8c6b40051ba3125c84298ee44ca52f5d17..f805aa73f3aa244d847149eec26505181ce4efe8 100644
> --- a/gcc/dumpfile.cc
> +++ b/gcc/dumpfile.cc
> @@ -63,6 +63,7 @@ FILE *dump_file = NULL;
>  const char *dump_file_name;
>  dump_flags_t dump_flags;
>  bool dumps_are_enabled = false;
> +bool dump_folding_p = false;
>
>
>  /* Set global "dump_file" to NEW_DUMP_FILE, refreshing the "dumps_are_enabled"
> @@ -73,6 +74,7 @@ set_dump_file (FILE *new_dump_file)
>  {
>    dumpfile_ensure_any_optinfo_are_flushed ();
>    dump_file = new_dump_file;
> +  dump_folding_p = dump_file && (dump_flags & TDF_FOLDING);
>    dump_context::get ().refresh_dumps_are_enabled ();
>  }
>
> diff --git a/gcc/generic-match-head.cc b/gcc/generic-match-head.cc
> index f011204c5be450663231bdece0596317b37f9f9b..16b8f9f3b61d3d5651a5a41a8c0552f50b55cc7c 100644
> --- a/gcc/generic-match-head.cc
> +++ b/gcc/generic-match-head.cc
> @@ -102,3 +102,17 @@ optimize_successive_divisions_p (tree, tree)
>  {
>    return false;
>  }
> +
> +/* Helper method for debug printing to reducing string parsing overhead.  Keep
> +   in sync with version in gimple-match-head.cc.  */
> +
> +static
> +void dump_debug (bool simplify, int loc, const char *file, int lineno)
> +{
> +  if (simplify)
> +    fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n", "match.pd", loc,
> +            file, lineno);
> +  else
> +    fprintf (dump_file, "Matching expression %s:%d, %s:%d\n", "match.pd", loc,
> +            file, lineno);
> +}
> \ No newline at end of file
> diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
> index 638606b2502f640e59527fc5a0b23fa3bedd0cee..bd7c6ff4a3fb89d456b02242707fd823b737f20d 100644
> --- a/gcc/genmatch.cc
> +++ b/gcc/genmatch.cc
> @@ -185,7 +185,8 @@ fprintf_indent (FILE *f, unsigned int indent, const char *format, ...)
>
>  static void
>  output_line_directive (FILE *f, location_t location,
> -                      bool dumpfile = false, bool fnargs = false)
> +                      bool dumpfile = false, bool fnargs = false,
> +                      bool loc_only = false)
>  {
>    const line_map_ordinary *map;
>    linemap_resolve_location (line_table, location, LRK_SPELLING_LOCATION, &map);
> @@ -204,7 +205,9 @@ output_line_directive (FILE *f, location_t location,
>        else
>         ++file;
>
> -      if (fnargs)
> +      if (loc_only)
> +       fprintf (f, "%d", loc.line);
> +      else if (fnargs)
>         fprintf (f, "\"%s\", %d", file, loc.line);
>        else
>         fprintf (f, "%s:%d", file, loc.line);
> @@ -3431,14 +3434,11 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple, operand *result)
>        needs_label = true;
>      }
>
> -  fprintf_indent (f, indent, "if (UNLIKELY (dump_file && (dump_flags & TDF_FOLDING))) "
> -          "fprintf (dump_file, \"%s ",
> -          s->kind == simplify::SIMPLIFY
> -          ? "Applying pattern" : "Matching expression");
> -  fprintf (f, "%%s:%%d, %%s:%%d\\n\", ");
> +  fprintf_indent (f, indent, "if (UNLIKELY (dump_folding_p)) "
> +       "dump_debug (%s, ", s->kind == simplify::SIMPLIFY ? "true" : "false");
>    output_line_directive (f,
>                          result ? result->location : s->match->location, true,
> -                        true);
> +                        true, true);
>    fprintf (f, ", __FILE__, __LINE__);\n");
>
>    fprintf_indent (f, indent, "{\n");
> diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
> index ec603f9d043c3924ea442bb49b5300a3573503cf..ae0c5c8a74fd9f1acdb616014941b11961e96c04 100644
> --- a/gcc/gimple-match-head.cc
> +++ b/gcc/gimple-match-head.cc
> @@ -1412,3 +1412,17 @@ get_conditional_internal_fn (code_helper code, tree type)
>    auto cfn = combined_fn (code);
>    return get_conditional_internal_fn (associated_internal_fn (cfn, type));
>  }
> +
> +/* Helper method for debug printing to reducing string parsing overhead.  Keep
> +   in sync with version in generic-match-head.cc.  */
> +
> +static
> +void dump_debug (bool simplify, int loc, const char *file, int lineno)
> +{
> +  if (simplify)
> +    fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n", "match.pd", loc,
> +            file, lineno);
> +  else
> +    fprintf (dump_file, "Matching expression %s:%d, %s:%d\n", "match.pd", loc,
> +            file, lineno);
> +}
> \ No newline at end of file
>
>
>
>
> --