From: Richard Biener <rguenther@suse.de>
To: Tamar Christina <Tamar.Christina@arm.com>
Cc: Richard Biener <richard.guenther@gmail.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
nd <nd@arm.com>, "jlaw@ventanamicro.com" <jlaw@ventanamicro.com>
Subject: RE: [PATCH 2/3]middle-end match.pd: simplify debug dump checks
Date: Tue, 25 Apr 2023 13:13:34 +0000 (UTC) [thread overview]
Message-ID: <nycvar.YFH.7.77.849.2304251311160.4466@jbgna.fhfr.qr> (raw)
In-Reply-To: <VI1PR08MB5325DC0FC3CCF835DFD066BFFF649@VI1PR08MB5325.eurprd08.prod.outlook.com>
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.
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)
next prev parent reply other threads:[~2023-04-25 13:13 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
2023-04-25 12:30 ` Tamar Christina
2023-04-25 13:13 ` Richard Biener [this message]
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=nycvar.YFH.7.77.849.2304251311160.4466@jbgna.fhfr.qr \
--to=rguenther@suse.de \
--cc=Tamar.Christina@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jlaw@ventanamicro.com \
--cc=nd@arm.com \
--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).