| Message ID | 0ccfec07-234b-d354-7ca6-9e83984708a3@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | genmatch: isolate reporting into a function | expand |
On Mon, Sep 3, 2018 at 9:56 AM Martin Liška <mliska@suse.cz> wrote: > > Hi. > > This is small improvement for {gimple,generic}-match.c files. > The code path that reports application of a pattern is now guarded > with __builtin_expect. And reporting function lives in gimple.c. > > For gimple-match.o, difference is: > > bloaty /tmp/after.o -- /tmp/before.o > VM SIZE FILE SIZE > ++++++++++++++ GROWING ++++++++++++++ > [ = ] 0 .rela.debug_loc +58.5Ki +0.5% > +0.7% +7.70Ki .text +7.70Ki +0.7% > [ = ] 0 .debug_info +3.53Ki +0.6% > [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% > [ = ] 0 .debug_loc +1.86Ki +0.7% > +0.7% +448 .eh_frame +448 +0.7% > [ = ] 0 .rela.eh_frame +192 +0.7% > [ = ] 0 .rela.debug_line +48 +0.4% > [ = ] 0 .debug_str +26 +0.0% > +6.9% +9 .rodata.str1.1 +9 +6.9% > > -------------- SHRINKING -------------- > -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% > [ = ] 0 .symtab -14.7Ki -26.1% > [ = ] 0 .strtab -3.57Ki -2.2% > [ = ] 0 .rela.debug_info -2.81Ki -0.0% > [ = ] 0 .debug_line -2.14Ki -0.6% > [ = ] 0 .rela.text -816 -0.1% > [ = ] 0 .rela.text.unlikely -288 -0.1% > -0.1% -131 .text.unlikely -131 -0.1% > [ = ] 0 [Unmapped] -23 -14.0% > [ = ] 0 .debug_abbrev -2 -0.1% > > -1.2% -16.8Ki TOTAL +25.1Ki +0.1% > > I'm testing the patch. > Thoughts? Looks good in principle but why have the function in gimple.c rather than in gimple-match-head.c where it could be static? Do we still end up inlining it even though it is guarded with __builtin_expect? Richard. > > Martin > > gcc/ChangeLog: > > 2018-08-31 Martin Liska <mliska@suse.cz> > > * genmatch.c (output_line_directive): Add new argument > fnargs. > (dt_simplify::gen_1): Generate call to report_match_pattern > function and wrap that into __builtin_expect. > * gimple.c (report_match_pattern): New function. > * gimple.h (report_match_pattern): Likewise. > --- > gcc/genmatch.c | 18 ++++++++++++------ > gcc/gimple.c | 14 ++++++++++++++ > gcc/gimple.h | 4 ++++ > 3 files changed, 30 insertions(+), 6 deletions(-) > >
On 09/03/2018 10:19 AM, Richard Biener wrote: > On Mon, Sep 3, 2018 at 9:56 AM Martin Liška <mliska@suse.cz> wrote: >> >> Hi. >> >> This is small improvement for {gimple,generic}-match.c files. >> The code path that reports application of a pattern is now guarded >> with __builtin_expect. And reporting function lives in gimple.c. >> >> For gimple-match.o, difference is: >> >> bloaty /tmp/after.o -- /tmp/before.o >> VM SIZE FILE SIZE >> ++++++++++++++ GROWING ++++++++++++++ >> [ = ] 0 .rela.debug_loc +58.5Ki +0.5% >> +0.7% +7.70Ki .text +7.70Ki +0.7% >> [ = ] 0 .debug_info +3.53Ki +0.6% >> [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% >> [ = ] 0 .debug_loc +1.86Ki +0.7% >> +0.7% +448 .eh_frame +448 +0.7% >> [ = ] 0 .rela.eh_frame +192 +0.7% >> [ = ] 0 .rela.debug_line +48 +0.4% >> [ = ] 0 .debug_str +26 +0.0% >> +6.9% +9 .rodata.str1.1 +9 +6.9% >> >> -------------- SHRINKING -------------- >> -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% >> [ = ] 0 .symtab -14.7Ki -26.1% >> [ = ] 0 .strtab -3.57Ki -2.2% >> [ = ] 0 .rela.debug_info -2.81Ki -0.0% >> [ = ] 0 .debug_line -2.14Ki -0.6% >> [ = ] 0 .rela.text -816 -0.1% >> [ = ] 0 .rela.text.unlikely -288 -0.1% >> -0.1% -131 .text.unlikely -131 -0.1% >> [ = ] 0 [Unmapped] -23 -14.0% >> [ = ] 0 .debug_abbrev -2 -0.1% >> >> -1.2% -16.8Ki TOTAL +25.1Ki +0.1% >> >> I'm testing the patch. >> Thoughts? > > Looks good in principle but why have the function in gimple.c > rather than in gimple-match-head.c where it could be static? > Do we still end up inlining it even though it is guarded with > __builtin_expect? Done that transformation: #include "gimple-match-head.c" static void report_match_pattern (const char *match_file, unsigned int match_file_line, const char *generated_file, unsigned int generate_file_line) { fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n",match_file, match_file_line, generated_file, generate_file_line); } Yes, I can confirm it's inlined now. Ready to install after proper testing? Martin > > Richard. > >> >> Martin >> >> gcc/ChangeLog: >> >> 2018-08-31 Martin Liska <mliska@suse.cz> >> >> * genmatch.c (output_line_directive): Add new argument >> fnargs. >> (dt_simplify::gen_1): Generate call to report_match_pattern >> function and wrap that into __builtin_expect. >> * gimple.c (report_match_pattern): New function. >> * gimple.h (report_match_pattern): Likewise. >> --- >> gcc/genmatch.c | 18 ++++++++++++------ >> gcc/gimple.c | 14 ++++++++++++++ >> gcc/gimple.h | 4 ++++ >> 3 files changed, 30 insertions(+), 6 deletions(-) >> >> From 79c11daed376e1e157d0e7d867d690524f9df686 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Fri, 31 Aug 2018 16:23:35 +0200 Subject: [PATCH] genmatch: isolate reporting into a function gcc/ChangeLog: 2018-08-31 Martin Liska <mliska@suse.cz> * genmatch.c (output_line_directive): Add new argument fnargs. (write_header): Generate static void report_match_pattern. (dt_simplify::gen_1): Generate call to report_match_pattern function and wrap that into __builtin_expect. * genmatch.c: --- gcc/genmatch.c | 29 ++++++++++++++++++++++------- gcc/gimple.h | 1 - 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/gcc/genmatch.c b/gcc/genmatch.c index 50d72f8f1e7..b73ba1bc8e9 100644 --- a/gcc/genmatch.c +++ b/gcc/genmatch.c @@ -184,7 +184,7 @@ fprintf_indent (FILE *f, unsigned int indent, const char *format, ...) static void output_line_directive (FILE *f, source_location location, - bool dumpfile = false) + bool dumpfile = false, bool fnargs = false) { const line_map_ordinary *map; linemap_resolve_location (line_table, location, LRK_SPELLING_LOCATION, &map); @@ -202,7 +202,11 @@ output_line_directive (FILE *f, source_location location, file = loc.file; else ++file; - fprintf (f, "%s:%d", file, loc.line); + + if (fnargs) + fprintf (f, "\"%s\", %d", file, loc.line); + else + fprintf (f, "%s:%d", file, loc.line); } else /* Other gen programs really output line directives here, at least for @@ -3305,11 +3309,13 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple, operand *result) } } - fprintf_indent (f, indent, "if (dump_file && (dump_flags & TDF_FOLDING)) " - "fprintf (dump_file, \"Applying pattern "); + fprintf_indent (f, indent, "if (__builtin_expect (dump_file && " + "(dump_flags & TDF_FOLDING), 0)) report_match_pattern ("); + output_line_directive (f, - result ? result->location : s->match->location, true); - fprintf (f, ", %%s:%%d\\n\", __FILE__, __LINE__);\n"); + result ? result->location : s->match->location, true, + true); + fprintf (f, ", __FILE__, __LINE__);\n"); if (!result) { @@ -3888,8 +3894,17 @@ write_header (FILE *f, const char *head) /* Include the header instead of writing it awkwardly quoted here. */ fprintf (f, "\n#include \"%s\"\n", head); -} + /* Generate report_match_pattern function. */ + fprintf (f, "static void report_match_pattern (const char *match_file, " + "unsigned int match_file_line, " + "const char *generated_file, " + "unsigned int generate_file_line)\n"); + fprintf (f, "{\n"); + fprintf (f, "fprintf (dump_file, \"Applying pattern %%s:%%d, %%s:%%d\\n\"," + "match_file, match_file_line, generated_file, generate_file_line);\n"); + fprintf (f, "}\n"); +} /* AST parsing. */ diff --git a/gcc/gimple.h b/gcc/gimple.h index a5dda9369bc..2e1de5d5490 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -6404,7 +6404,6 @@ gimple_set_do_not_emit_location (gimple *g) gimple_set_plf (g, GF_PLF_1, true); } - /* Macros for showing usage statistics. */ #define SCALE(x) ((unsigned long) ((x) < 1024*10 \ ? (x) \
On Mon, Sep 3, 2018 at 2:09 PM Martin Liška <mliska@suse.cz> wrote: > > On 09/03/2018 10:19 AM, Richard Biener wrote: > > On Mon, Sep 3, 2018 at 9:56 AM Martin Liška <mliska@suse.cz> wrote: > >> > >> Hi. > >> > >> This is small improvement for {gimple,generic}-match.c files. > >> The code path that reports application of a pattern is now guarded > >> with __builtin_expect. And reporting function lives in gimple.c. > >> > >> For gimple-match.o, difference is: > >> > >> bloaty /tmp/after.o -- /tmp/before.o > >> VM SIZE FILE SIZE > >> ++++++++++++++ GROWING ++++++++++++++ > >> [ = ] 0 .rela.debug_loc +58.5Ki +0.5% > >> +0.7% +7.70Ki .text +7.70Ki +0.7% > >> [ = ] 0 .debug_info +3.53Ki +0.6% > >> [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% > >> [ = ] 0 .debug_loc +1.86Ki +0.7% > >> +0.7% +448 .eh_frame +448 +0.7% > >> [ = ] 0 .rela.eh_frame +192 +0.7% > >> [ = ] 0 .rela.debug_line +48 +0.4% > >> [ = ] 0 .debug_str +26 +0.0% > >> +6.9% +9 .rodata.str1.1 +9 +6.9% > >> > >> -------------- SHRINKING -------------- > >> -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% > >> [ = ] 0 .symtab -14.7Ki -26.1% > >> [ = ] 0 .strtab -3.57Ki -2.2% > >> [ = ] 0 .rela.debug_info -2.81Ki -0.0% > >> [ = ] 0 .debug_line -2.14Ki -0.6% > >> [ = ] 0 .rela.text -816 -0.1% > >> [ = ] 0 .rela.text.unlikely -288 -0.1% > >> -0.1% -131 .text.unlikely -131 -0.1% > >> [ = ] 0 [Unmapped] -23 -14.0% > >> [ = ] 0 .debug_abbrev -2 -0.1% > >> > >> -1.2% -16.8Ki TOTAL +25.1Ki +0.1% > >> > >> I'm testing the patch. > >> Thoughts? > > > > Looks good in principle but why have the function in gimple.c > > rather than in gimple-match-head.c where it could be static? > > Do we still end up inlining it even though it is guarded with > > __builtin_expect? > > Done that transformation: > > #include "gimple-match-head.c" > static void report_match_pattern (const char *match_file, unsigned int match_file_line, const char *generated_file, unsigned int generate_file_line) > { > fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n",match_file, match_file_line, generated_file, generate_file_line); > } > > Yes, I can confirm it's inlined now. Hmm, but that was the point of the exercise? Not inlining it? Or was the point to have the __builtin_expect()? > Ready to install after proper testing? Just occured to me you need a copy of that in generic-match-head.c. But then, why not just add the __builtin_expect()... > Martin > > > > > Richard. > > > >> > >> Martin > >> > >> gcc/ChangeLog: > >> > >> 2018-08-31 Martin Liska <mliska@suse.cz> > >> > >> * genmatch.c (output_line_directive): Add new argument > >> fnargs. > >> (dt_simplify::gen_1): Generate call to report_match_pattern > >> function and wrap that into __builtin_expect. > >> * gimple.c (report_match_pattern): New function. > >> * gimple.h (report_match_pattern): Likewise. > >> --- > >> gcc/genmatch.c | 18 ++++++++++++------ > >> gcc/gimple.c | 14 ++++++++++++++ > >> gcc/gimple.h | 4 ++++ > >> 3 files changed, 30 insertions(+), 6 deletions(-) > >> > >> >
On 09/03/2018 03:31 PM, Richard Biener wrote: > On Mon, Sep 3, 2018 at 2:09 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 09/03/2018 10:19 AM, Richard Biener wrote: >>> On Mon, Sep 3, 2018 at 9:56 AM Martin Liška <mliska@suse.cz> wrote: >>>> >>>> Hi. >>>> >>>> This is small improvement for {gimple,generic}-match.c files. >>>> The code path that reports application of a pattern is now guarded >>>> with __builtin_expect. And reporting function lives in gimple.c. >>>> >>>> For gimple-match.o, difference is: >>>> >>>> bloaty /tmp/after.o -- /tmp/before.o >>>> VM SIZE FILE SIZE >>>> ++++++++++++++ GROWING ++++++++++++++ >>>> [ = ] 0 .rela.debug_loc +58.5Ki +0.5% >>>> +0.7% +7.70Ki .text +7.70Ki +0.7% >>>> [ = ] 0 .debug_info +3.53Ki +0.6% >>>> [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% >>>> [ = ] 0 .debug_loc +1.86Ki +0.7% >>>> +0.7% +448 .eh_frame +448 +0.7% >>>> [ = ] 0 .rela.eh_frame +192 +0.7% >>>> [ = ] 0 .rela.debug_line +48 +0.4% >>>> [ = ] 0 .debug_str +26 +0.0% >>>> +6.9% +9 .rodata.str1.1 +9 +6.9% >>>> >>>> -------------- SHRINKING -------------- >>>> -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% >>>> [ = ] 0 .symtab -14.7Ki -26.1% >>>> [ = ] 0 .strtab -3.57Ki -2.2% >>>> [ = ] 0 .rela.debug_info -2.81Ki -0.0% >>>> [ = ] 0 .debug_line -2.14Ki -0.6% >>>> [ = ] 0 .rela.text -816 -0.1% >>>> [ = ] 0 .rela.text.unlikely -288 -0.1% >>>> -0.1% -131 .text.unlikely -131 -0.1% >>>> [ = ] 0 [Unmapped] -23 -14.0% >>>> [ = ] 0 .debug_abbrev -2 -0.1% >>>> >>>> -1.2% -16.8Ki TOTAL +25.1Ki +0.1% >>>> >>>> I'm testing the patch. >>>> Thoughts? >>> >>> Looks good in principle but why have the function in gimple.c >>> rather than in gimple-match-head.c where it could be static? >>> Do we still end up inlining it even though it is guarded with >>> __builtin_expect? >> >> Done that transformation: >> >> #include "gimple-match-head.c" >> static void report_match_pattern (const char *match_file, unsigned int match_file_line, const char *generated_file, unsigned int generate_file_line) >> { >> fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n",match_file, match_file_line, generated_file, generate_file_line); >> } >> >> Yes, I can confirm it's inlined now. > > Hmm, but that was the point of the exercise? Not inlining it? Or was > the point to have > the __builtin_expect()? The point was __builtin_expect and I thought I can also save some space. > >> Ready to install after proper testing? > > Just occured to me you need a copy of that in generic-match-head.c. > > But then, why not just add the __builtin_expect()... Yes, let's add that. And it's questionable whether to split the string in: gimple_seq *lseq = seq; - if (dump_file && (dump_flags & TDF_FOLDING)) fprintf (dump_file, "Applying pattern match.pd:4858, %s:%d\n", __FILE__, __LINE__); + if (__builtin_expect (dump_file && (dump_flags & TDF_FOLDING), 0)) fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n", "match.pd", 4858, __FILE__, __LINE__); res_op->set_op (CFN_FNMA, type, 3); That does: bloaty /tmp/after.o -- /tmp/before.o VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ [ = ] 0 .rela.text +22.7Ki +3.5% +1.6% +17.8Ki .text +17.8Ki +1.6% [ = ] 0 .rela.debug_ranges +3.89Ki +0.0% [ = ] 0 .debug_info +3.08Ki +0.5% +1.8% +1.09Ki .eh_frame +1.09Ki +1.8% [ = ] 0 .debug_loc +1.01Ki +0.4% [ = ] 0 .rela.eh_frame +480 +1.9% +0.1% +195 .text.unlikely +195 +0.1% [ = ] 0 .rela.debug_line +72 +0.6% +6.9% +9 .rodata.str1.1 +9 +6.9% [ = ] 0 .debug_ranges +1 +0.0% -------------- SHRINKING -------------- -97.4% -24.8Ki .rodata.str1.8 -24.8Ki -97.4% [ = ] 0 .rela.debug_loc -14.5Ki -0.1% [ = ] 0 .symtab -14.4Ki -25.6% [ = ] 0 .rela.debug_info -3.45Ki -0.0% [ = ] 0 .strtab -2.48Ki -1.5% [ = ] 0 .debug_line -2.43Ki -0.7% [ = ] 0 [Unmapped] -15 -9.1% [ = ] 0 .debug_abbrev -12 -0.6% -0.4% -5.68Ki TOTAL -11.7Ki -0.0% It's up to you. Martin > >> Martin >> >>> >>> Richard. >>> >>>> >>>> Martin >>>> >>>> gcc/ChangeLog: >>>> >>>> 2018-08-31 Martin Liska <mliska@suse.cz> >>>> >>>> * genmatch.c (output_line_directive): Add new argument >>>> fnargs. >>>> (dt_simplify::gen_1): Generate call to report_match_pattern >>>> function and wrap that into __builtin_expect. >>>> * gimple.c (report_match_pattern): New function. >>>> * gimple.h (report_match_pattern): Likewise. >>>> --- >>>> gcc/genmatch.c | 18 ++++++++++++------ >>>> gcc/gimple.c | 14 ++++++++++++++ >>>> gcc/gimple.h | 4 ++++ >>>> 3 files changed, 30 insertions(+), 6 deletions(-) >>>> >>>> >> From 031b662cfb4afd9e5612d19ea6d61eb22b014c6d Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Fri, 31 Aug 2018 16:23:35 +0200 Subject: [PATCH] genmatch: put reporting on a cold path gcc/ChangeLog: 2018-09-03 Martin Liska <mliska@suse.cz> * genmatch.c (output_line_directive): Add new argument fnargs. (dt_simplify::gen_1): Encapsulate dump within __builtin_expect. --- gcc/genmatch.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/gcc/genmatch.c b/gcc/genmatch.c index 50d72f8f1e7..5f1691ae206 100644 --- a/gcc/genmatch.c +++ b/gcc/genmatch.c @@ -184,7 +184,7 @@ fprintf_indent (FILE *f, unsigned int indent, const char *format, ...) static void output_line_directive (FILE *f, source_location location, - bool dumpfile = false) + bool dumpfile = false, bool fnargs = false) { const line_map_ordinary *map; linemap_resolve_location (line_table, location, LRK_SPELLING_LOCATION, &map); @@ -202,7 +202,11 @@ output_line_directive (FILE *f, source_location location, file = loc.file; else ++file; - fprintf (f, "%s:%d", file, loc.line); + + if (fnargs) + fprintf (f, "\"%s\", %d", file, loc.line); + else + fprintf (f, "%s:%d", file, loc.line); } else /* Other gen programs really output line directives here, at least for @@ -3305,11 +3309,13 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple, operand *result) } } - fprintf_indent (f, indent, "if (dump_file && (dump_flags & TDF_FOLDING)) " + fprintf_indent (f, indent, "if (__builtin_expect (dump_file && (dump_flags & TDF_FOLDING), 0)) " "fprintf (dump_file, \"Applying pattern "); + fprintf (f, "%%s:%%d, %%s:%%d\\n\", "); output_line_directive (f, - result ? result->location : s->match->location, true); - fprintf (f, ", %%s:%%d\\n\", __FILE__, __LINE__);\n"); + result ? result->location : s->match->location, true, + true); + fprintf (f, ", __FILE__, __LINE__);\n"); if (!result) {
On Mon, Sep 3, 2018 at 4:19 PM Martin Liška <mliska@suse.cz> wrote: > > On 09/03/2018 03:31 PM, Richard Biener wrote: > > On Mon, Sep 3, 2018 at 2:09 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 09/03/2018 10:19 AM, Richard Biener wrote: > >>> On Mon, Sep 3, 2018 at 9:56 AM Martin Liška <mliska@suse.cz> wrote: > >>>> > >>>> Hi. > >>>> > >>>> This is small improvement for {gimple,generic}-match.c files. > >>>> The code path that reports application of a pattern is now guarded > >>>> with __builtin_expect. And reporting function lives in gimple.c. > >>>> > >>>> For gimple-match.o, difference is: > >>>> > >>>> bloaty /tmp/after.o -- /tmp/before.o > >>>> VM SIZE FILE SIZE > >>>> ++++++++++++++ GROWING ++++++++++++++ > >>>> [ = ] 0 .rela.debug_loc +58.5Ki +0.5% > >>>> +0.7% +7.70Ki .text +7.70Ki +0.7% > >>>> [ = ] 0 .debug_info +3.53Ki +0.6% > >>>> [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% > >>>> [ = ] 0 .debug_loc +1.86Ki +0.7% > >>>> +0.7% +448 .eh_frame +448 +0.7% > >>>> [ = ] 0 .rela.eh_frame +192 +0.7% > >>>> [ = ] 0 .rela.debug_line +48 +0.4% > >>>> [ = ] 0 .debug_str +26 +0.0% > >>>> +6.9% +9 .rodata.str1.1 +9 +6.9% > >>>> > >>>> -------------- SHRINKING -------------- > >>>> -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% > >>>> [ = ] 0 .symtab -14.7Ki -26.1% > >>>> [ = ] 0 .strtab -3.57Ki -2.2% > >>>> [ = ] 0 .rela.debug_info -2.81Ki -0.0% > >>>> [ = ] 0 .debug_line -2.14Ki -0.6% > >>>> [ = ] 0 .rela.text -816 -0.1% > >>>> [ = ] 0 .rela.text.unlikely -288 -0.1% > >>>> -0.1% -131 .text.unlikely -131 -0.1% > >>>> [ = ] 0 [Unmapped] -23 -14.0% > >>>> [ = ] 0 .debug_abbrev -2 -0.1% > >>>> > >>>> -1.2% -16.8Ki TOTAL +25.1Ki +0.1% > >>>> > >>>> I'm testing the patch. > >>>> Thoughts? > >>> > >>> Looks good in principle but why have the function in gimple.c > >>> rather than in gimple-match-head.c where it could be static? > >>> Do we still end up inlining it even though it is guarded with > >>> __builtin_expect? > >> > >> Done that transformation: > >> > >> #include "gimple-match-head.c" > >> static void report_match_pattern (const char *match_file, unsigned int match_file_line, const char *generated_file, unsigned int generate_file_line) > >> { > >> fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n",match_file, match_file_line, generated_file, generate_file_line); > >> } > >> > >> Yes, I can confirm it's inlined now. > > > > Hmm, but that was the point of the exercise? Not inlining it? Or was > > the point to have > > the __builtin_expect()? > > The point was __builtin_expect and I thought I can also save some space. > > > > >> Ready to install after proper testing? > > > > Just occured to me you need a copy of that in generic-match-head.c. > > > > But then, why not just add the __builtin_expect()... > > Yes, let's add that. And it's questionable whether to split the string in: > > gimple_seq *lseq = seq; > - if (dump_file && (dump_flags & TDF_FOLDING)) fprintf (dump_file, "Applying pattern match.pd:4858, %s:%d\n", __FILE__, __LINE__); > + if (__builtin_expect (dump_file && (dump_flags & TDF_FOLDING), 0)) fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n", "match.pd", 4858, __FILE__, __LINE__); > res_op->set_op (CFN_FNMA, type, 3); I guess it makes sense to split it, so the patch is OK. Richard. > That does: > > bloaty /tmp/after.o -- /tmp/before.o > VM SIZE FILE SIZE > ++++++++++++++ GROWING ++++++++++++++ > [ = ] 0 .rela.text +22.7Ki +3.5% > +1.6% +17.8Ki .text +17.8Ki +1.6% > [ = ] 0 .rela.debug_ranges +3.89Ki +0.0% > [ = ] 0 .debug_info +3.08Ki +0.5% > +1.8% +1.09Ki .eh_frame +1.09Ki +1.8% > [ = ] 0 .debug_loc +1.01Ki +0.4% > [ = ] 0 .rela.eh_frame +480 +1.9% > +0.1% +195 .text.unlikely +195 +0.1% > [ = ] 0 .rela.debug_line +72 +0.6% > +6.9% +9 .rodata.str1.1 +9 +6.9% > [ = ] 0 .debug_ranges +1 +0.0% > > -------------- SHRINKING -------------- > -97.4% -24.8Ki .rodata.str1.8 -24.8Ki -97.4% > [ = ] 0 .rela.debug_loc -14.5Ki -0.1% > [ = ] 0 .symtab -14.4Ki -25.6% > [ = ] 0 .rela.debug_info -3.45Ki -0.0% > [ = ] 0 .strtab -2.48Ki -1.5% > [ = ] 0 .debug_line -2.43Ki -0.7% > [ = ] 0 [Unmapped] -15 -9.1% > [ = ] 0 .debug_abbrev -12 -0.6% > > -0.4% -5.68Ki TOTAL -11.7Ki -0.0% > > It's up to you. > > Martin > > > > >> Martin > >> > >>> > >>> Richard. > >>> > >>>> > >>>> Martin > >>>> > >>>> gcc/ChangeLog: > >>>> > >>>> 2018-08-31 Martin Liska <mliska@suse.cz> > >>>> > >>>> * genmatch.c (output_line_directive): Add new argument > >>>> fnargs. > >>>> (dt_simplify::gen_1): Generate call to report_match_pattern > >>>> function and wrap that into __builtin_expect. > >>>> * gimple.c (report_match_pattern): New function. > >>>> * gimple.h (report_match_pattern): Likewise. > >>>> --- > >>>> gcc/genmatch.c | 18 ++++++++++++------ > >>>> gcc/gimple.c | 14 ++++++++++++++ > >>>> gcc/gimple.h | 4 ++++ > >>>> 3 files changed, 30 insertions(+), 6 deletions(-) > >>>> > >>>> > >> >
On 9/3/18 1:56 AM, Martin Liška wrote: > Hi. > > This is small improvement for {gimple,generic}-match.c files. > The code path that reports application of a pattern is now guarded > with __builtin_expect. And reporting function lives in gimple.c. > > For gimple-match.o, difference is: > > bloaty /tmp/after.o -- /tmp/before.o > VM SIZE FILE SIZE > ++++++++++++++ GROWING ++++++++++++++ > [ = ] 0 .rela.debug_loc +58.5Ki +0.5% > +0.7% +7.70Ki .text +7.70Ki +0.7% > [ = ] 0 .debug_info +3.53Ki +0.6% > [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% > [ = ] 0 .debug_loc +1.86Ki +0.7% > +0.7% +448 .eh_frame +448 +0.7% > [ = ] 0 .rela.eh_frame +192 +0.7% > [ = ] 0 .rela.debug_line +48 +0.4% > [ = ] 0 .debug_str +26 +0.0% > +6.9% +9 .rodata.str1.1 +9 +6.9% > > -------------- SHRINKING -------------- > -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% > [ = ] 0 .symtab -14.7Ki -26.1% > [ = ] 0 .strtab -3.57Ki -2.2% > [ = ] 0 .rela.debug_info -2.81Ki -0.0% > [ = ] 0 .debug_line -2.14Ki -0.6% > [ = ] 0 .rela.text -816 -0.1% > [ = ] 0 .rela.text.unlikely -288 -0.1% > -0.1% -131 .text.unlikely -131 -0.1% > [ = ] 0 [Unmapped] -23 -14.0% > [ = ] 0 .debug_abbrev -2 -0.1% > > -1.2% -16.8Ki TOTAL +25.1Ki +0.1% > > I'm testing the patch. > Thoughts? > > Martin > > gcc/ChangeLog: > > 2018-08-31 Martin Liska <mliska@suse.cz> > > * genmatch.c (output_line_directive): Add new argument > fnargs. > (dt_simplify::gen_1): Generate call to report_match_pattern > function and wrap that into __builtin_expect. > * gimple.c (report_match_pattern): New function. > * gimple.h (report_match_pattern): Likewise. Seems reasonable, though I'm not generally a fan of __builtin_expect :-) jeff
diff --git a/gcc/genmatch.c b/gcc/genmatch.c index 50d72f8f1e7..8e209b7e6cb 100644 --- a/gcc/genmatch.c +++ b/gcc/genmatch.c @@ -184,7 +184,7 @@ fprintf_indent (FILE *f, unsigned int indent, const char *format, ...) static void output_line_directive (FILE *f, source_location location, - bool dumpfile = false) + bool dumpfile = false, bool fnargs = false) { const line_map_ordinary *map; linemap_resolve_location (line_table, location, LRK_SPELLING_LOCATION, &map); @@ -202,7 +202,11 @@ output_line_directive (FILE *f, source_location location, file = loc.file; else ++file; - fprintf (f, "%s:%d", file, loc.line); + + if (fnargs) + fprintf (f, "\"%s\", %d", file, loc.line); + else + fprintf (f, "%s:%d", file, loc.line); } else /* Other gen programs really output line directives here, at least for @@ -3305,11 +3309,13 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple, operand *result) } } - fprintf_indent (f, indent, "if (dump_file && (dump_flags & TDF_FOLDING)) " - "fprintf (dump_file, \"Applying pattern "); + fprintf_indent (f, indent, "if (__builtin_expect (dump_file && " + "(dump_flags & TDF_FOLDING), 0)) report_match_pattern ("); + output_line_directive (f, - result ? result->location : s->match->location, true); - fprintf (f, ", %%s:%%d\\n\", __FILE__, __LINE__);\n"); + result ? result->location : s->match->location, true, + true); + fprintf (f, ", __FILE__, __LINE__);\n"); if (!result) { diff --git a/gcc/gimple.c b/gcc/gimple.c index e3e651b1e61..7f4254c930d 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -3190,6 +3190,20 @@ gimple_inexpensive_call_p (gcall *stmt) return false; } +/* Report about a match of pattern that lives + in MATCH_FILE at MATCH_FILE_LINE. The pattern is matched + in GENERATED_FILE at line GENERATE_FILE_LINE. */ + +void report_match_pattern (const char *match_file, + unsigned int match_file_line, + const char *generated_file, + unsigned int generate_file_line) +{ + fprintf (dump_file, "Applying pattern %s:%d, %s:%d\n", + match_file, match_file_line, + generated_file, generate_file_line); +} + #if CHECKING_P namespace selftest { diff --git a/gcc/gimple.h b/gcc/gimple.h index a5dda9369bc..cd9e3e54e79 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -6404,6 +6404,10 @@ gimple_set_do_not_emit_location (gimple *g) gimple_set_plf (g, GF_PLF_1, true); } +extern void report_match_pattern (const char *match_file, + unsigned int match_file_line, + const char *generated_file, + unsigned int generate_file_line); /* Macros for showing usage statistics. */ #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
Hi. This is small improvement for {gimple,generic}-match.c files. The code path that reports application of a pattern is now guarded with __builtin_expect. And reporting function lives in gimple.c. For gimple-match.o, difference is: bloaty /tmp/after.o -- /tmp/before.o VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ [ = ] 0 .rela.debug_loc +58.5Ki +0.5% +0.7% +7.70Ki .text +7.70Ki +0.7% [ = ] 0 .debug_info +3.53Ki +0.6% [ = ] 0 .rela.debug_ranges +2.02Ki +0.0% [ = ] 0 .debug_loc +1.86Ki +0.7% +0.7% +448 .eh_frame +448 +0.7% [ = ] 0 .rela.eh_frame +192 +0.7% [ = ] 0 .rela.debug_line +48 +0.4% [ = ] 0 .debug_str +26 +0.0% +6.9% +9 .rodata.str1.1 +9 +6.9% -------------- SHRINKING -------------- -97.5% -24.8Ki .rodata.str1.8 -24.8Ki -97.5% [ = ] 0 .symtab -14.7Ki -26.1% [ = ] 0 .strtab -3.57Ki -2.2% [ = ] 0 .rela.debug_info -2.81Ki -0.0% [ = ] 0 .debug_line -2.14Ki -0.6% [ = ] 0 .rela.text -816 -0.1% [ = ] 0 .rela.text.unlikely -288 -0.1% -0.1% -131 .text.unlikely -131 -0.1% [ = ] 0 [Unmapped] -23 -14.0% [ = ] 0 .debug_abbrev -2 -0.1% -1.2% -16.8Ki TOTAL +25.1Ki +0.1% I'm testing the patch. Thoughts? Martin gcc/ChangeLog: 2018-08-31 Martin Liska <mliska@suse.cz> * genmatch.c (output_line_directive): Add new argument fnargs. (dt_simplify::gen_1): Generate call to report_match_pattern function and wrap that into __builtin_expect. * gimple.c (report_match_pattern): New function. * gimple.h (report_match_pattern): Likewise. --- gcc/genmatch.c | 18 ++++++++++++------ gcc/gimple.c | 14 ++++++++++++++ gcc/gimple.h | 4 ++++ 3 files changed, 30 insertions(+), 6 deletions(-)