On Mon, Sep 3, 2018 at 4:19 PM Martin Liška <mli...@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 <mli...@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 <mli...@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 <mli...@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(-)
> >>>>
> >>>>
> >>
>
