> 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?
Kyrill
> 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");
0001-genoutput-write-each-output-template-out-once.patch
Description: 0001-genoutput-write-each-output-template-out-once.patch
