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);
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(-)
>>>>
>>>>
>>
>From 031b662cfb4afd9e5612d19ea6d61eb22b014c6d Mon Sep 17 00:00:00 2001
From: marxin <mli...@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 <mli...@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)
{
--
2.18.0
