From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 1CA943858D1E for ; Tue, 25 Apr 2023 13:13:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1CA943858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 464BD1FD9D; Tue, 25 Apr 2023 13:13:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1682428414; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=35lH4Ex/f5zTsLJiBB80468wcHZESjQiBUP/yTcDPNs=; b=hysQhcScdH45MsvnVIi/llHalBKHQz3YxXHDm6kxfO+WZuGwnkULI4uTzbR4T+N1GEqi6U aJ8xLA2XBZKFPwntUDKa84ksbpp6bywR/azX3si71it+SKskS0e6y3DlXubOsCsLbuIlTo RCExNwVgGWAcUkAyNBZW7vheKpp2+0M= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1682428414; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=35lH4Ex/f5zTsLJiBB80468wcHZESjQiBUP/yTcDPNs=; b=lCr5BxqwVBQhwEMou2+F5oi6VK81twOC8NOHOtqFntNq6AfCuv2J9IAsibA1IgrsvNQnVy vU0saTMSu2JmtEDQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 2AEC22C141; Tue, 25 Apr 2023 13:13:34 +0000 (UTC) Date: Tue, 25 Apr 2023 13:13:34 +0000 (UTC) From: Richard Biener To: Tamar Christina cc: Richard Biener , "gcc-patches@gcc.gnu.org" , nd , "jlaw@ventanamicro.com" Subject: RE: [PATCH 2/3]middle-end match.pd: simplify debug dump checks In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_LOTSOFHASH,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 25 Apr 2023, Tamar Christina wrote: > > -----Original Message----- > > From: Richard Biener > > Sent: Tuesday, April 18, 2023 11:48 AM > > To: Tamar Christina > > Cc: gcc-patches@gcc.gnu.org; nd ; 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 > 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 SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)