Kyrylo Tkachov <[email protected]> writes: >> On 9 Sep 2026, at 13:34, Richard Sandiford >> <[email protected]> wrote: >> >> 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, something like the attached?
OK, thanks. Good point about the n_alternatives check having to be repeated, but I think it does still work out better in the end. Richard
