public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Tamar Christina <Tamar.Christina@arm.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	nd <nd@arm.com>, "rguenther@suse.de" <rguenther@suse.de>,
	"jlaw@ventanamicro.com" <jlaw@ventanamicro.com>
Subject: RE: [PATCH 2/3]middle-end match.pd: simplify debug dump checks
Date: Wed, 19 Apr 2023 10:44:43 +0000	[thread overview]
Message-ID: <VI1PR08MB5325C0933BE19E0398FF9B06FF629@VI1PR08MB5325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <CAFiYyc2FaknVbw4ZL9MS9pyYvxHkO+zXV4VKfziJcUf-_pnYQg@mail.gmail.com>

> -----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
> *?

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
> >
> >
> >
> >
> > --

  reply	other threads:[~2023-04-19 10:44 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-06 10:56 [PATCH 1/3] RFC match.pd: don't emit label if not needed Tamar Christina
2023-04-06 10:56 ` [PATCH 2/3] RFC - match.pd: simplify debug dump checks Tamar Christina
2023-04-18 10:19 ` [PATCH 1/3]middle-end match.pd: don't emit label if not needed Tamar Christina
2023-04-18 10:20 ` [PATCH 2/3]middle-end match.pd: simplify debug dump checks Tamar Christina
2023-04-18 10:47   ` Richard Biener
2023-04-19 10:44     ` Tamar Christina [this message]
2023-04-25 12:30     ` Tamar Christina
2023-04-25 13:13       ` Richard Biener
2023-04-25 14:17         ` Tamar Christina
2023-04-18 10:20 ` [PATCH 3/3]middle-end RFC - match.pd: automatically partition *-match.cc files Tamar Christina
2023-04-19 11:18   ` Richard Biener
2023-04-28 10:43     ` Tamar Christina
2023-04-28 12:39       ` Richard Biener
2023-04-18 10:38 ` [PATCH 1/3]middle-end match.pd: don't emit label if not needed Richard Biener
2023-04-18 16:46   ` Tamar Christina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR08MB5325C0933BE19E0398FF9B06FF629@VI1PR08MB5325.eurprd08.prod.outlook.com \
    --to=tamar.christina@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jlaw@ventanamicro.com \
    --cc=nd@arm.com \
    --cc=rguenther@suse.de \
    --cc=richard.guenther@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).