On Thu, Aug 3, 2023 at 4:24 PM Andrzej Turko via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Currently fprintf calls logging to a dump file take line numbers
> in the match.pd file directly as arguments.
> When match.pd is edited, referenced code changes line numbers,
> which causes changes to many fprintf calls and, thus, to many
> (usually all) .cc files generated by genmatch. This forces make
> to (unnecessarily) rebuild many .o files.
>
> This change replaces those logging fprintf calls with calls to
> a dedicated logging function. Because it reads the line numbers
> from the lookup table, it is enough to pass a corresponding index.
> Thanks to this, when match.pd changes, it is enough to rebuild
> the file containing the lookup table and, of course, those
> actually affected by the change.
>
> Signed-off-by: Andrzej Turko <andrzej.tu...@gmail.com>
>
> gcc/ChangeLog:
>
> * genmatch.cc: Log line numbers indirectly.
> ---
> gcc/genmatch.cc | 89 ++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 74 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
> index 1deca505603..63d6ba6dab0 100644
> --- a/gcc/genmatch.cc
> +++ b/gcc/genmatch.cc
> @@ -217,9 +217,57 @@ fp_decl_done (FILE *f, const char *trailer)
> fprintf (header_file, "%s;", trailer);
> }
>
> +/* Line numbers for use by indirect line directives. */
> +static vec<int> dbg_line_numbers;
> +
> +static void
> +write_header_declarations (bool gimple, FILE *f)
> +{
> + fprintf (f, "\nextern void\n%s_dump_logs (const char *file1, int line1_id,
> "
> + "const char *file2, int line2, bool simplify);\n",
> + gimple ? "gimple" : "generic");
> +}
> +
> +static void
> +define_dump_logs (bool gimple, FILE *f)
> +{
> +
extra vertical space is unwanted here.
> + if (dbg_line_numbers.is_empty ())
> + {
> + fprintf (f, "};\n\n");
> + return;
> + }
shouldn't the above come after ...
> + fprintf (f , "void\n%s_dump_logs (const char *file1, int line1_id, "
> + "const char *file2, int line2, bool simplify)\n{\n",
> + gimple ? "gimple" : "generic");
... this?
> + fprintf_indent (f, 2, "static int dbg_line_numbers[%d] = {",
> + dbg_line_numbers.length ());
> +
> + for (int i = 0; i < (int)dbg_line_numbers.length () - 1; i++)
use an unsigned int to avoid the cast?
> + {
> + if (i % 20 == 0)
> + fprintf (f, "\n\t");
> +
> + fprintf (f, "%d, ", dbg_line_numbers[i]);
> + }
> + fprintf (f, "%d\n };\n\n", dbg_line_numbers.last ());
> +
> +
> + fprintf_indent (f, 2, "fprintf (dump_file, \"%%s "
> + "%%s:%%d, %%s:%%d\\n\",\n");
> + fprintf_indent (f, 10, "simplify ? \"Applying pattern\" : "
> + "\"Matching expression\", file1, "
> + "dbg_line_numbers[line1_id], file2, line2);");
> +
> + fprintf (f, "\n}\n\n");
> +}
> +
> static void
> output_line_directive (FILE *f, location_t location,
> - bool dumpfile = false, bool fnargs = false)
> + bool dumpfile = false, bool fnargs = false,
> + bool indirect_line_numbers = false)
> {
> const line_map_ordinary *map;
> linemap_resolve_location (line_table, location, LRK_SPELLING_LOCATION,
> &map);
> @@ -239,7 +287,15 @@ output_line_directive (FILE *f, location_t location,
> ++file;
>
> if (fnargs)
> - fprintf (f, "\"%s\", %d", file, loc.line);
> + {
> + if (indirect_line_numbers)
> + {
> + fprintf (f, "\"%s\", %d", file, dbg_line_numbers.length ());
> + dbg_line_numbers.safe_push (loc.line);
> + }
> + else
> + fprintf (f, "\"%s\", %d", file, loc.line);
> + }
The indent is off here. I notice the same lines often appear repeatedly so
a simple optimization like doing
if (indirect_line_numbers)
{
if (!dbg_line_numbers.is_empty ()
&& dbg_line_numbers.last () == loc.line)
;
else
dbg_line_numbers.safe_push (loc.line);
fprintf (f, "\"%s\", %d", file, dbg_line_numbers.length () - 1);
}
shrinks the table quite a bit (not all duplicates are gone this way).
It doesn't
seem we can easily keep the list sorted, adding another hash-map could
avoid duplicates completely, maybe worth pursuing.
Otherwise the patch series looks fine to me.
Thanks,
Richard.
> else
> fprintf (f, "%s:%d", file, loc.line);
> }
> @@ -3375,20 +3431,19 @@ dt_operand::gen (FILE *f, int indent, bool gimple,
> int depth)
> }
> }
>
> -/* Emit a fprintf to the debug file to the file F, with the INDENT from
> +/* Emit a logging call to the debug file to the file F, with the INDENT from
> either the RESULT location or the S's match location if RESULT is null. */
> static void
> -emit_debug_printf (FILE *f, int indent, class simplify *s, operand *result)
> +emit_logging_call (FILE *f, int indent, class simplify *s, operand *result,
> + bool gimple)
> {
> fprintf_indent (f, indent, "if (UNLIKELY (debug_dump)) "
> - "fprintf (dump_file, \"%s ",
> - s->kind == simplify::SIMPLIFY
> - ? "Applying pattern" : "Matching expression");
> - fprintf (f, "%%s:%%d, %%s:%%d\\n\", ");
> + "%s_dump_logs (", gimple ? "gimple" : "generic");
> output_line_directive (f,
> - result ? result->location : s->match->location, true,
> - true);
> - fprintf (f, ", __FILE__, __LINE__);\n");
> + result ? result->location : s->match->location,
> + true, true, true);
> + fprintf (f, ", __FILE__, __LINE__, %s);\n",
> + s->kind == simplify::SIMPLIFY ? "true" : "false");
> }
>
> /* Generate code for the '(if ...)', '(with ..)' and actual transform
> @@ -3524,7 +3579,7 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple,
> operand *result)
> if (!result)
> {
> /* If there is no result then this is a predicate implementation. */
> - emit_debug_printf (f, indent, s, result);
> + emit_logging_call (f, indent, s, result, gimple);
> fprintf_indent (f, indent, "return true;\n");
> }
> else if (gimple)
> @@ -3615,7 +3670,7 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple,
> operand *result)
> }
> else
> gcc_unreachable ();
> - emit_debug_printf (f, indent, s, result);
> + emit_logging_call (f, indent, s, result, gimple);
> fprintf_indent (f, indent, "return true;\n");
> }
> else /* GENERIC */
> @@ -3670,7 +3725,7 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple,
> operand *result)
> }
> if (is_predicate)
> {
> - emit_debug_printf (f, indent, s, result);
> + emit_logging_call (f, indent, s, result, gimple);
> fprintf_indent (f, indent, "return true;\n");
> }
> else
> @@ -3738,7 +3793,7 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple,
> operand *result)
> i);
> }
> }
> - emit_debug_printf (f, indent, s, result);
> + emit_logging_call (f, indent, s, result, gimple);
> fprintf_indent (f, indent, "return _r;\n");
> }
> }
> @@ -5447,6 +5502,7 @@ main (int argc, char **argv)
> parts.quick_push (stdout);
> write_header (stdout, s_include_file);
> write_header_includes (gimple, stdout);
> + write_header_declarations (gimple, stdout);
> }
> else
> {
> @@ -5460,6 +5516,7 @@ main (int argc, char **argv)
> fprintf (header_file, "#ifndef GCC_GIMPLE_MATCH_AUTO_H\n"
> "#define GCC_GIMPLE_MATCH_AUTO_H\n");
> write_header_includes (gimple, header_file);
> + write_header_declarations (gimple, header_file);
> }
>
> /* Go over all predicates defined with patterns and perform
> @@ -5502,6 +5559,8 @@ main (int argc, char **argv)
>
> dt.gen (parts, gimple);
>
> + define_dump_logs (gimple, choose_output (parts));
> +
> for (FILE *f : parts)
> {
> fprintf (f, "#pragma GCC diagnostic pop\n");
> --
> 2.34.1
>
