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 3BFBC3853563 for ; Fri, 28 Apr 2023 12:39:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3BFBC3853563 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 63AAD2008B; Fri, 28 Apr 2023 12:39:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1682685563; 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=S/S6W3SmmEChXa58IOiQPccYS544+TfldN9521bYwv8=; b=GCSwFcAGzZ3IVWx01SmP17g9WCLXpRmLfI9q23uU4h3bc2tXjYr7OvJlK1Bqd5U6mhpzdU WN7Pd85B3PTopVHHnFh4ErEVu57Mhm/UuZxhSfazUWbSJdp9mM+HBv7wm7dy5zzGAGQ5+W 59oe7hY3nsWygdqTIXfYU8yPLWdcJes= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1682685563; 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=S/S6W3SmmEChXa58IOiQPccYS544+TfldN9521bYwv8=; b=RIkjKq3X0GEFK6Q2Q7tdoUy6rFWib0S+dgjlc85rgGx0E1bEezFvloyaBqgmoBP4s5Y16/ ECW7ktA2RLUm9KAg== 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 509E82C18F; Fri, 28 Apr 2023 12:39:23 +0000 (UTC) Date: Fri, 28 Apr 2023 12:39:23 +0000 (UTC) From: Richard Biener To: Tamar Christina cc: Richard Biener , "gcc-patches@gcc.gnu.org" , nd , "jlaw@ventanamicro.com" Subject: RE: [PATCH 3/3]middle-end RFC - match.pd: automatically partition *-match.cc files. 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=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,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 Fri, 28 Apr 2023, Tamar Christina wrote: > > > [1] https://gcc.gnu.org/legacy-ml/gcc-patches/2018-04/msg01125.html > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > > > Ok for master? > > > > Some comments - I have to leave the Makefile bits to somebody else to see > > whether they are portable as-is. > > > > The private functions now in gimple-match-exports.cc are not supposed to be > > public API, so the additions to gimple-match.h should be avoided - can > > you add the declarations to gimple-match-head.cc instead? At least I don't > > see how the refactoring needs to add anything to gimple-match.h? > > > > -decision_tree::gen (FILE *f, bool gimple) > > +decision_tree::gen (FILE **files, int n_parts, bool gimple) > > > > can you use a vec<> please to avoid passing n_parts separately? > > > > + /* Set a default value for the tool to 5, but GCC itself uses > > + whatever default is determined by the configure variable > > + DEFAULT_MATCHPD_PARTITIONS. */ > > + int n_parts = 5; > > + char *input = argv[argc-2]; > > ... > > fprintf (stderr, "Usage: genmatch " > > - "[--gimple] [--generic] [-v[v]] input\n"); > > + "[--gimple] [--generic] [--splits=] [-v[v]] > > input outdir\n"); > > > > I don't like this - I'm using ./build/genmatch --gimple test.pd | less to debug > > genmatch changes with a small test input and like to preserve that. Can > > you instead change the usage to > > > > genmatch --gimple match.pd gimple-match-1.c gimple-match-2.c > > gimple-match-3.c ... > > > > thus > > > > - "[--gimple] [--generic] [-v[v]] input\n"); > > + "[--gimple] [--generic] [-v[v]] input [output...]\n"); > > > > and when no output is specified continue to use stdout? Possibly when > > more than one output is given require a --header outfile argument to > > specify the header file to use (and for one output make emit_func > > not ICE but instead not emit to the header, aka header_file == NULL?). > > Ideally without makefile changes that would produce the same > > gimple-match.cc as before (minus the -head.cc changes of course). > > > > The gimple-match-head.cc/exports changes could be split out as > > far as I can see? Likewise the Makefile changes if the argument > > control is changed as I sugggest? > > > > All changes done. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR bootstrap/84402 > * genmatch.cc (emit_func, SIZED_BASED_CHUNKS, get_out_file): New. > (decision_tree::gen): Accept list of files instead of single and update > to write function definition to header and main file. > (write_predicate): Likewise. > (write_header): Emit pragmas and new includes. > (main): Create file buffers and cleanup. > (showUsage): New. > > --- inline copy of patch --- > > diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc > index 716fb97aac4c3c2baae82e068df3ce158b9afee9..f56b4bc992d87cb7d707e59be2d61c44a45b68e6 100644 > --- a/gcc/genmatch.cc > +++ b/gcc/genmatch.cc > @@ -183,6 +183,33 @@ fprintf_indent (FILE *f, unsigned int indent, const char *format, ...) > va_end (ap); > } > > +/* Like fprintf, but print to two files, one header one C implementation. */ > +FILE *header_file = NULL; > + > +static void > +#if GCC_VERSION >= 4001 > +__attribute__((format (printf, 4, 5))) > +#endif > +emit_func (FILE *f, bool open, bool close, const char *format, ...) > +{ > + va_list ap1, ap2; > + if (header_file != stdout) would making this header_file == NULL also work? > + { > + if (open) > + fprintf (header_file, "extern "); > + va_start (ap2, format); > + vfprintf (header_file, format, ap2); > + va_end (ap2); > + if (close) > + fprintf (header_file, ";\n"); > + } > + > + va_start (ap1, format); > + vfprintf (f, format, ap1); > + va_end (ap1); > + fputc ('\n', f); > +} > + > static void > output_line_directive (FILE *f, location_t location, > bool dumpfile = false, bool fnargs = false) > @@ -217,6 +244,34 @@ output_line_directive (FILE *f, location_t location, > fprintf (f, "/* #line %d \"%s\" */\n", loc.line, loc.file); > } > > +/* Find the file to write into next. We try to evenly distribute the contents > + over the different files. */ > + > +#define SIZED_BASED_CHUNKS 1 > + > +int current_file = 0; > +FILE *get_out_file (vec &parts) > +{ > +#ifdef SIZED_BASED_CHUNKS > + FILE *f = NULL; > + long min = 0; > + /* We've started writing all the files at pos 0, so ftell is equivalent > + to the size and should be much faster. */ > + for (unsigned i = 0; i < parts.length (); i++) > + { > + long res = ftell (parts[i]); Looks good to me, but I wonder what this will do if the single part is 'stdout' - I think ftell will error (return -1) and set errno to ESPIPE? I'd say special-casing parts.length () == 1 would make sense here? > + if (!f || res < min) > + { > + min = res; > + f = parts[i]; > + } > + } > + return f; > +#else > + return parts[current_file++ % parts.length ()]; > +#endif > +} > + > > /* Pull in tree codes and builtin function codes from their > definition files. */ > @@ -1732,7 +1787,7 @@ public: > dt_node *root; > > void insert (class simplify *, unsigned); > - void gen (FILE *f, bool gimple); > + void gen (vec &f, bool gimple); > void print (FILE *f = stderr); > > decision_tree () { root = new dt_node (dt_node::DT_NODE, NULL); } > @@ -3830,7 +3885,7 @@ sinfo_hashmap_traits::equal_keys (const key_type &v, > tree. */ > > void > -decision_tree::gen (FILE *f, bool gimple) > +decision_tree::gen (vec &files, bool gimple) > { > sinfo_map_t si; > > @@ -3859,11 +3914,14 @@ decision_tree::gen (FILE *f, bool gimple) > output_line_directive (stderr, s->s->s->result->location); > } > > + /* Cycle the file buffers. */ > + FILE *f = get_out_file (files); > + > /* Generate a split out function with the leaf transform code. */ > s->fname = xasprintf ("%s_simplify_%u", gimple ? "gimple" : "generic", > fcnt++); > if (gimple) > - fprintf (f, "\nstatic bool\n" > + emit_func (f, true, false, "\nbool\n" > "%s (gimple_match_op *res_op, gimple_seq *seq,\n" > " tree (*valueize)(tree) ATTRIBUTE_UNUSED,\n" > " const tree ARG_UNUSED (type), tree *ARG_UNUSED " > @@ -3871,27 +3929,28 @@ decision_tree::gen (FILE *f, bool gimple) > s->fname); > else > { > - fprintf (f, "\nstatic tree\n" > + emit_func (f, true, false, "\ntree\n" > "%s (location_t ARG_UNUSED (loc), const tree ARG_UNUSED (type),\n", > (*iter).second->fname); > for (unsigned i = 0; > i < as_a (s->s->s->match)->ops.length (); ++i) > - fprintf (f, " tree ARG_UNUSED (_p%d),", i); > - fprintf (f, " tree *captures\n"); > + emit_func (f, false, false, " tree ARG_UNUSED (_p%d),", i); > + emit_func (f, false, false, " tree *captures\n"); > } > for (unsigned i = 0; i < s->s->s->for_subst_vec.length (); ++i) > { > if (! s->s->s->for_subst_vec[i].first->used) > continue; > if (is_a (s->s->s->for_subst_vec[i].second)) > - fprintf (f, ", const enum tree_code ARG_UNUSED (%s)", > + emit_func (f, false, false, ", const enum tree_code ARG_UNUSED (%s)", > s->s->s->for_subst_vec[i].first->id); > else if (is_a (s->s->s->for_subst_vec[i].second)) > - fprintf (f, ", const combined_fn ARG_UNUSED (%s)", > + emit_func (f, false, false, ", const combined_fn ARG_UNUSED (%s)", > s->s->s->for_subst_vec[i].first->id); > } > > - fprintf (f, ")\n{\n"); > + emit_func (f, false, true, ")"); > + fprintf (f, "{\n"); > fprintf_indent (f, 2, "const bool debug_dump = " > "dump_file && (dump_flags & TDF_FOLDING);\n"); > s->s->gen_1 (f, 2, gimple, s->s->s->result); > @@ -3921,8 +3980,12 @@ decision_tree::gen (FILE *f, bool gimple) > && e->operation->kind != id_base::CODE)) > continue; > > + > + /* Cycle the file buffers. */ > + FILE *f = get_out_file (files); > + > if (gimple) > - fprintf (f, "\nstatic bool\n" > + emit_func (f, true, false,"\nbool\n" > "gimple_simplify_%s (gimple_match_op *res_op," > " gimple_seq *seq,\n" > " tree (*valueize)(tree) " > @@ -3931,13 +3994,13 @@ decision_tree::gen (FILE *f, bool gimple) > "ARG_UNUSED (type)\n", > e->operation->id); > else > - fprintf (f, "\nstatic tree\n" > + emit_func (f, true, false, "\ntree\n" > "generic_simplify_%s (location_t ARG_UNUSED (loc), enum " > "tree_code ARG_UNUSED (code), const tree ARG_UNUSED (type)", > e->operation->id); > for (unsigned i = 0; i < n; ++i) > - fprintf (f, ", tree _p%d", i); > - fprintf (f, ")\n"); > + emit_func (f, false, false,", tree _p%d", i); > + emit_func (f, false, true, ")"); > fprintf (f, "{\n"); > fprintf_indent (f, 2, "const bool debug_dump = " > "dump_file && (dump_flags & TDF_FOLDING);\n"); > @@ -3954,18 +4017,22 @@ decision_tree::gen (FILE *f, bool gimple) > with compiler warnings, by generating a simple stub. */ > if (! has_kids_p) > { > + > + /* Cycle the file buffers. */ > + FILE *f = get_out_file (files); > + > if (gimple) > - fprintf (f, "\nbool\n" > + emit_func (f, true, false, "\nbool\n" > "gimple_simplify (gimple_match_op*, gimple_seq*,\n" > " tree (*)(tree), code_helper,\n" > " const tree"); > else > - fprintf (f, "\ntree\n" > + emit_func (f, true, false,"\ntree\n" > "generic_simplify (location_t, enum tree_code,\n" > " const tree"); > for (unsigned i = 0; i < n; ++i) > - fprintf (f, ", tree"); > - fprintf (f, ")\n"); > + emit_func (f, false, false, ", tree"); > + emit_func (f, false, true, ")"); > fprintf (f, "{\n"); > if (gimple) > fprintf (f, " return false;\n"); > @@ -3975,20 +4042,24 @@ decision_tree::gen (FILE *f, bool gimple) > continue; > } > > + > + /* Cycle the file buffers. */ > + FILE *f = get_out_file (files); > + > /* Then generate the main entry with the outermost switch and > tail-calls to the split-out functions. */ > if (gimple) > - fprintf (f, "\nbool\n" > + emit_func (f, true, false, "\nbool\n" > "gimple_simplify (gimple_match_op *res_op, gimple_seq *seq,\n" > " tree (*valueize)(tree) ATTRIBUTE_UNUSED,\n" > " code_helper code, const tree type"); > else > - fprintf (f, "\ntree\n" > + emit_func (f, true, false, "\ntree\n" > "generic_simplify (location_t loc, enum tree_code code, " > "const tree type ATTRIBUTE_UNUSED"); > for (unsigned i = 0; i < n; ++i) > - fprintf (f, ", tree _p%d", i); > - fprintf (f, ")\n"); > + emit_func (f, false, false, ", tree _p%d", i); > + emit_func (f, false, true, ")"); > fprintf (f, "{\n"); > > if (gimple) > @@ -4043,11 +4114,11 @@ decision_tree::gen (FILE *f, bool gimple) > void > write_predicate (FILE *f, predicate_id *p, decision_tree &dt, bool gimple) > { > - fprintf (f, "\nbool\n" > - "%s%s (tree t%s%s)\n" > - "{\n", gimple ? "gimple_" : "tree_", p->id, > - p->nargs > 0 ? ", tree *res_ops" : "", > - gimple ? ", tree (*valueize)(tree) ATTRIBUTE_UNUSED" : ""); > + emit_func (f, true, true, "\nbool\n%s%s (tree t%s%s)", > + gimple ? "gimple_" : "tree_", p->id, > + p->nargs > 0 ? ", tree *res_ops" : "", > + gimple ? ", tree (*valueize)(tree) ATTRIBUTE_UNUSED" : ""); > + fprintf (f, "{\n"); > /* Conveniently make 'type' available. */ > fprintf_indent (f, 2, "const tree type = TREE_TYPE (t);\n"); > fprintf_indent (f, 2, "const bool debug_dump = " > @@ -4068,9 +4139,13 @@ write_header (FILE *f, const char *head) > { > fprintf (f, "/* Generated automatically by the program `genmatch' from\n"); > fprintf (f, " a IL pattern matching and simplification description. */\n"); > + fprintf (f, "#pragma GCC diagnostic push\n"); > + fprintf (f, "#pragma GCC diagnostic ignored \"-Wunused-variable\"\n"); > + fprintf (f, "#pragma GCC diagnostic ignored \"-Wunused-function\"\n"); > > /* Include the header instead of writing it awkwardly quoted here. */ > - fprintf (f, "\n#include \"%s\"\n", head); > + if (head) > + fprintf (f, "\n#include \"%s\"\n", head); > } > > > @@ -5213,6 +5288,16 @@ round_alloc_size (size_t s) > } > > > +static void > +showUsage () > +{ > + fprintf (stderr, "Usage: genmatch [--gimple] [--generic] " > + "[--header=] [--include=] [-v[v]] input " > + "[...]\n"); > + fprintf (stderr, "\nWhen more then one outputfile is specified --header " > + "is required.\n"); > +} > + > /* The genmatch generator program. It reads from a pattern description > and outputs GIMPLE or GENERIC IL matching and simplification routines. */ > > @@ -5228,25 +5313,44 @@ main (int argc, char **argv) > > bool gimple = true; > verbose = 0; > - char *input = argv[argc-1]; > - for (int i = 1; i < argc - 1; ++i) > + char *s_header_file = NULL; > + char *s_include_file = NULL; > + auto_vec files; > + char *input = NULL; > + int last_file = argc - 1; > + for (int i = argc - 1; i >= 1; --i) > { > if (strcmp (argv[i], "--gimple") == 0) > gimple = true; > else if (strcmp (argv[i], "--generic") == 0) > gimple = false; > + else if (strncmp (argv[i], "--header=", 9) == 0) > + s_header_file = &argv[i][9]; > + else if (strncmp (argv[i], "--include=", 10) == 0) > + s_include_file = &argv[i][10]; > else if (strcmp (argv[i], "-v") == 0) > verbose = 1; > else if (strcmp (argv[i], "-vv") == 0) > verbose = 2; > + else if (strncmp (argv[i], "--", 2) != 0 && last_file-- == i) > + files.safe_push (argv[i]); > else > { > - fprintf (stderr, "Usage: genmatch " > - "[--gimple] [--generic] [-v[v]] input\n"); > + showUsage (); > return 1; > } > } > > + /* Validate if the combinations are valid. */ > + if ((files.length () > 1 && !s_header_file) || files.is_empty ()) > + showUsage (); > + > + if (!s_include_file) > + s_include_file = s_header_file; > + > + /* Input file is the last in the reverse list. */ > + input = files.pop (); > + > line_table = XCNEW (class line_maps); > linemap_init (line_table, 0); > line_table->reallocator = xrealloc; > @@ -5293,10 +5397,32 @@ main (int argc, char **argv) > /* Parse ahead! */ > parser p (r, gimple); > > + /* Create file buffers. */ > + int n_parts = files.is_empty () ? 1 : files.length (); > + auto_vec parts (n_parts); > + if (files.is_empty ()) > + { > + parts.quick_push (stdout); > + header_file = stdout; = NULL would be more natural? > + write_header (stdout, s_include_file); > + } > + else > + { > + for (char *s_file : files) > + { > + parts.quick_push (fopen (s_file, "w")); > + write_header (parts.last (), s_include_file); > + } > + > + header_file = fopen (s_header_file, "w"); > + fprintf (header_file, "#ifndef GCC_GIMPLE_MATCH_AUTO_H\n" > + "#define GCC_GIMPLE_MATCH_AUTO_H\n"); > + } > + > if (gimple) > - write_header (stdout, "gimple-match-head.cc"); > + fprintf (header_file, "#include \"gimple-match-head.cc\"\n"); > else > - write_header (stdout, "generic-match-head.cc"); > + fprintf (header_file, "#include \"generic-match-head.cc\"\n"); > > /* Go over all predicates defined with patterns and perform > lowering and code generation. */ > @@ -5316,7 +5442,10 @@ main (int argc, char **argv) > if (verbose == 2) > dt.print (stderr); > > - write_predicate (stdout, pred, dt, gimple); > + /* Cycle the file buffers. */ > + FILE *f = get_out_file (parts); > + > + write_predicate (f, pred, dt, gimple); > } > > /* Lower the main simplifiers and generate code for them. */ > @@ -5333,7 +5462,19 @@ main (int argc, char **argv) > if (verbose == 2) > dt.print (stderr); > > - dt.gen (stdout, gimple); > + dt.gen (parts, gimple); > + > + for (FILE *f : parts) > + { > + fprintf (f, "#pragma GCC diagnostic pop\n"); > + fclose (f); > + } > + > + if (!files.is_empty ()) likewise then !header_file. Otherwise looks OK. Thanks, Richard. > + { > + fprintf (header_file, "#endif /* GCC_GIMPLE_MATCH_AUTO_H. */\n"); > + fclose (header_file); > + } > > /* Finalize. */ > cpp_finish (r, NULL);