> -----Original Message-----
> From: Richard Biener <richard.guent...@gmail.com>
> Sent: Tuesday, April 18, 2023 11:48 AM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; rguent...@suse.de;
> j...@ventanamicro.com
> Subject: Re: [PATCH 2/3]middle-end match.pd: simplify debug dump checks
>
> On Tue, Apr 18, 2023 at 12:22 PM Tamar Christina via Gcc-patches <gcc-
> patc...@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
> *?
Yeah that would work too, will do.
>
> 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.
Sure, I'll hold up respinning waiting on the 3rd patch review since this one
will change
that one as well, so easier to handle all comments at once.
Thanks for the review,
Tamar
>
> 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..e7b595ddecdcca9983d958
> 4b8b24
> > 17ae1941c7d4 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..f805aa73f3aa244d84714
> 9eec265
> > 05181ce4efe8 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..16b8f9f3b61d3d5651a5
> a41a8c05
> > 52f50b55cc7c 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..bd7c6ff4a3fb89d456b022
> 42707f
> > d823b737f20d 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..ae0c5c8a74fd9f1acdb616
> 014941
> > b11961e96c04 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
> >
> >
> >
> >
> > --
