Sorry for the slow review. I didn't notice this until your recent(ish) ping.
<[email protected]> writes: > From: Kyrylo Tkachov <[email protected]> > > genoutput gives every pattern that needs one its own output_N definition, > whether or not an identical one has already been written. Iterators are > what make that expensive: one define_insn expanded over a mode iterator > produces a pattern per mode, and where the template does not mention the > mode, every one of those patterns gets the same definition. > > Two patterns produce identical definitions exactly when their > templates agree and come from the same place in the machine description, > since the location is what print_md_ptr_loc turns into the #line. Key on > both, and have the second pattern name the first rather than repeat it. > > On aarch64 the output_N definitions drop from 7990 to 3850. The > multi-alternative tables account for most of that, 6305 to 2524; the > functions go from 1685 to 1326. insn-output.cc goes from 7966037 bytes > in 125441 lines to 6777272 in 97657, and compiles in 18% less time, > with peak memory 424MB against 498MB. > > Keying on the location as well as the text means the #line in a shared > definition still names the pattern it came from, so debug information and > any host compiler diagnostic about the template are unchanged. > > Bootstrapped on aarch64-none-linux-gnu. > Ok for trunk? > > gcc/ChangeLog: > > * genoutput.cc (data): Add output_code. > (output_codes): New variable. > (process_template): Reuse an identical definition emitted earlier, > and record output_code. Check every pattern against its template > even when the definition is reused. > (output_insn_data): Use output_code. > > Signed-off-by: Kyrylo Tkachov <[email protected]> Thanks for doing this. I like the idea, but a suggestion below. > [...] > @@ -648,25 +648,60 @@ place_operands (class data *d) > It is either the assembler code template, a list of assembler code > templates, or C code to generate the assembler code template. */ > > +/* The output_* definitions emitted so far, keyed by the template that > + produced them together with the .md location that template came from. > + Two patterns agreeing on both get byte-identical definitions, down to the > + #line, so the second can name the first rather than repeat it. */ > + > +static hash_map<nofree_string_hash, int> output_codes; > + > static void > process_template (class data *d, const char *template_code) > { > const char *cp; > int i; > > + d->output_code = d->code_number; > + > + /* A template that needs a definition of its own may already have had one > + emitted for an earlier pattern. Every pattern is still checked against > + its template below, since the checks read the pattern rather than the > + template alone. Only the definition is emitted once. */ > + bool emit = true; > + if (template_code[0] == '*' || template_code[0] == '@') > + { > + const md_reader::ptr_loc *loc > + = rtx_reader_ptr->get_md_ptr_loc (template_code); > + char *key = xasprintf ("%s:%d\n%s", loc ? loc->loc.filename : "", > + loc ? loc->loc.lineno : 0, template_code); > + bool existed; > + int &prev = output_codes.get_or_insert (key, &existed); > + if (existed) > + { > + free (key); > + d->output_code = prev; > + emit = false; > + } > + else > + prev = d->code_number; > + } > + > /* Templates starting with * contain straight code to be run. */ > if (template_code[0] == '*') > { > d->template_code = 0; > d->output_format = INSN_OUTPUT_FORMAT_FUNCTION; > > - puts ("\nstatic const char *"); > - printf ("output_%d (rtx *operands ATTRIBUTE_UNUSED, rtx_insn *insn > ATTRIBUTE_UNUSED)\n", > - d->code_number); > - puts ("{"); > - rtx_reader_ptr->print_md_ptr_loc (template_code); > - puts (template_code + 1); > - puts ("}"); > + if (emit) > + { > + puts ("\nstatic const char *"); > + printf ("output_%d (rtx *operands ATTRIBUTE_UNUSED, rtx_insn *insn > ATTRIBUTE_UNUSED)\n", > + d->code_number); > + puts ("{"); > + rtx_reader_ptr->print_md_ptr_loc (template_code); > + puts (template_code + 1); > + puts ("}"); > + } > } > > /* If the assembler code template starts with a @ it is a newline-separated > @@ -688,17 +723,21 @@ process_template (class data *d, const char > *template_code) > if (found_star) > { > d->output_format = INSN_OUTPUT_FORMAT_FUNCTION; > - puts ("\nstatic const char *"); > - printf ("output_%d (rtx *operands ATTRIBUTE_UNUSED, " > - "rtx_insn *insn ATTRIBUTE_UNUSED)\n", d->code_number); > - puts ("{"); > - puts (" switch (which_alternative)\n {"); > + if (emit) > + { > + puts ("\nstatic const char *"); > + printf ("output_%d (rtx *operands ATTRIBUTE_UNUSED, " > + "rtx_insn *insn ATTRIBUTE_UNUSED)\n", d->code_number); > + puts ("{"); > + puts (" switch (which_alternative)\n {"); > + } > } > else > { > d->output_format = INSN_OUTPUT_FORMAT_MULTI; > - printf ("\nstatic const char * const output_%d[] = {\n", > - d->code_number); > + if (emit) > + printf ("\nstatic const char * const output_%d[] = {\n", > + d->code_number); > } > > for (i = 0, cp = &template_code[1]; *cp; ) Rather than have all this code conditional on "emit" (above and below), could we instead structure things as follows: /* Templates starting with * contain straight code to be run. */ if (template_code[0] == '*') { d->template_code = 0; d->output_format = INSN_OUTPUT_FORMAT_FUNCTION; } /* If the assembler code template starts with a @ it is a newline-separated list of assembler code templates, one for each alternative. */ else if (template_code[0] == '@') { d->template_code = 0; d->output_format = INSN_OUTPUT_FORMAT_MULTI; for (cp = &template_code[1]; *cp; ) { while (ISSPACE (*cp)) cp++; if (*cp == '*') { d->output_format = INSN_OUTPUT_FORMAT_FUNCTION; break; } while (!IS_VSPACE (*cp) && *cp != '\0') ++cp; } } else { d->template_code = template_code; d->output_format = INSN_OUTPUT_FORMAT_SINGLE; } (just pulling existing code further up, not co-authorship). Then the new code could be conditional on: if (!d->template_code) or similar and it could exit early if there is an existing match. There's no point repeating the error checking if the string has already been checked before. The remaining code could then use the output_format calculated above, rather than retesting template_code. Thanks, Richard > @@ -711,16 +750,18 @@ process_template (class data *d, const char > *template_code) > bp = cp; > if (found_star) > { > - printf (" case %d:", i); > + if (emit) > + printf (" case %d:", i); > if (*cp == '*') > { > - printf ("\n "); > + if (emit) > + printf ("\n "); > cp++; > } > - else > + else if (emit) > printf (" return \""); > } > - else > + else if (emit) > printf (" \""); > > for (ep = sp = cp; !IS_VSPACE (*ep) && *ep != '\0'; ++ep) > @@ -739,8 +780,11 @@ process_template (class data *d, const char > *template_code) > { > if (*p == '\\' && p + 1 < sp) > { > - putchar (*p); > - putchar (*(p+1)); > + if (emit) > + { > + putchar (*p); > + putchar (*(p+1)); > + } > p += 2; > continue; > } > @@ -754,7 +798,8 @@ process_template (class data *d, const char > *template_code) > else if (*p == '<' || *p == '>') > last_bracket = p; > > - putchar (*p); > + if (emit) > + putchar (*p); > p += 1; > } > > @@ -767,7 +812,7 @@ process_template (class data *d, const char > *template_code) > "error in iterator syntax in %s", cp); > } > } > - else > + else if (emit) > { > while (cp < sp) > putchar (*(cp++)); > @@ -775,7 +820,9 @@ process_template (class data *d, const char > *template_code) > > cp = sp; > > - if (!found_star) > + if (!emit) > + ; > + else if (!found_star) > puts ("\","); > else if (*bp != '*') > puts ("\";"); > @@ -796,7 +843,9 @@ process_template (class data *d, const char > *template_code) > error_at (d->loc, "wrong number of alternatives in the output" > " template"); > > - if (found_star) > + if (!emit) > + ; > + else if (found_star) > puts (" default: gcc_unreachable ();\n }\n}"); > else > printf ("};\n");
