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

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Richard Biener <rguenther@suse.de>
> Sent: Tuesday, April 25, 2023 2:14 PM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: Richard Biener <richard.guenther@gmail.com>; gcc-patches@gcc.gnu.org;
> nd <nd@arm.com>; jlaw@ventanamicro.com
> Subject: RE: [PATCH 2/3]middle-end match.pd: simplify debug dump checks
> 
> On Tue, 25 Apr 2023, Tamar Christina wrote:
> 
> > > -----Original Message-----
> > > From: Richard Biener <richard.guenther@gmail.com>
> > > Sent: Tuesday, April 18, 2023 11:48 AM
> > > To: Tamar Christina <Tamar.Christina@arm.com>
> > > Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; rguenther@suse.de;
> > > jlaw@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- 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 *?
> >
> > So the dumpfile itself is currently also a global, I did try wiring
> > this down, but the problem here is that eventually at the very top
> > level, I need to modify the gimple_simplify calls because of the overloads
> being created by the output, which also need the parameter.
> >
> > There things become interesting because this then conflicts with the
> > definitions in gimple-fold which would also need to take an additional
> argument and it breaks the public API.
> >
> > Wiring the dbg value through all the generated and public function
> > requires quite a lot of changes so I'm not sure this is worth it in that case.
> Should I just drop it?
> 
> Yeah, just drop it then.  Btw, I wasn't suggesting to pass it down from the
> actual API but compute it once (locally) at the toplevel entries in the generated
> gimple-match.cc and pass it down from there.  So we'd retain one
> (unconditionally done ...)
> 
>   FILE *local_dump_file = (dump_flags & TDF_folding) ? dump_file : NULL;
> 
> and pass that down to functions called.

Oohhhh apologies, that makes much more sense! I could have indeed stopped at
gimple-match.cc and compute there.  Fair enough. Let me make the change.

Thanks!,
Tamar

> 
> Richard.
> 
> 
> > Thanks,
> > Tamar
> >
> > >
> > > 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..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
> > > >
> > > >
> > > >
> > > >
> > > > --
> >
> 
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461
> Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald,
> Boudien Moerman; HRB 36809 (AG Nuernberg)