Re: [PATCH 8/9] Introduce class function_reader (v4)
On 12/01/2016 10:43 PM, David Malcolm wrote: + rtx_insn *jump_insn = get_insn_by_uid (1); + ASSERT_EQ (JUMP_INSN, GET_CODE (jump_insn)); + ASSERT_EQ (ret_rtx, JUMP_LABEL (jump_insn)); + // FIXME: ^^^ use ASSERT_RTX_PTR_EQ here ^^^ Why is this a fixme and not just done that way (several instances)? ASSERT_RTX_PTR_EQ doesn't exist yet; there was some discussion about it in earlier versions of the patch, but I haven't written it. It would be equivalent to ASSERT_EQ, checking pointer equality, but would dump the mismatching expected vs actual rtx on failure. Should I convert this to a TODO, or go ahead and implement ASSERT_RTX_PTR_EQ? Missed this question. Just add ASSERT_RTX_PTR_EQ, that shouldn't be hard, should it? Bernd
Re: [PATCH 8/9] Introduce class function_reader (v4)
On Thu, 2016-12-01 at 15:40 +0100, Bernd Schmidt wrote:
Thanks for looking at this.
> On 11/11/2016 10:15 PM, David Malcolm wrote:
> > #include "gt-aarch64.h"
> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > index 6c608e0..0dda786 100644
> > --- a/gcc/config/i386/i386.c
> > +++ b/gcc/config/i386/i386.c
>
> I think we should separate out the target specific tests so as to
> give
> port maintainers a chance to comment on them separately.
Done.
> > diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> > index 50cd388..179a91f 100644
> > --- a/gcc/emit-rtl.c
> > +++ b/gcc/emit-rtl.c
> > @@ -1371,6 +1371,19 @@ maybe_set_first_label_num (rtx_code_label
> > *x)
> >if (CODE_LABEL_NUMBER (x) < first_label_num)
> > first_label_num = CODE_LABEL_NUMBER (x);
> > }
> > +
> > +/* For use by the RTL function loader, when mingling with normal
> > + functions.
>
> Not sure what this means.
This is for patch 9 which adds __RTL support to cc1, so that we can
have testcases in which some function bodies are in RTL form, and
others are in normal C form.
The issue is that the CODE_LABEL_NUMBER values in the RTL dump make it
into the generated assembler, and must be unique within the .s file.
Hence we need to update first_label_num so that any new labels created
(e.g. when expanding pure C functions) don't clash with the ones from
the dump.
I can simply drop that first sentence; the followup probably is
sufficient:
/* Ensure that label_num is greater than the label num of X, to
avoid duplicate labels in the generated assembler. */
>
> >if (str == 0)
> > - fputs (" \"\"", m_outfile);
> > + fputs (" (nil)", m_outfile);
> >else
> > fprintf (m_outfile, " (\"%s\")", str);
> >m_sawclose = 1;
>
> What does this affect?
This affects things like the LABEL_NAME of CODE_LABEL instances.
The hunk means that we preserve NULL string values as NULL, rather than
them becoming the empty string on a round trip.
> > /* Global singleton; constrast with md_reader_ptr above. */
> > diff --git a/gcc/read-rtl-function.c b/gcc/read-rtl-function.c
> > new file mode 100644
> > index 000..ff6c808
> > --- /dev/null
> > +++ b/gcc/read-rtl-function.c
> > @@ -0,0 +1,2124 @@
> > +/* read-rtl-function.c - Reader for RTL function dumps
> > + Copyright (C) 2016 Free Software Foundation, Inc.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it
> > under
> > +the terms of the GNU General Public License as published by the
> > Free
> > +#include
>
> Please double-check all these includes whether they are necessary.
Thanks; yes, quite a few were unnecessary. Fixed.
> > +
> > +/* Fix up a NOTE_INSN_BASIC_BLOCK based on an integer block ID.
> > */
> > +
> > +void
> > +fixup_note_insn_basic_block::apply (function_reader */*reader*/)
> > const
>
> Lose the /*reader*/, probably.
Done.
> > +
> > +/* Implementation of rtx_reader::handle_unknown_directive.
> > +
> > + Require a top-level "function" elements, as emitted by
> > + print_rtx_function, and parse it. */
>
> "element"?
Converted to "directive" (I've been trying to use "directive" rather
than "element" throughout).
> > +void
> > +function_reader::create_function ()
> > +{
> > + /* Currently we assume cfgrtl mode, rather than cfglayout mode.
> > */
> > + if (0)
> > +cfg_layout_rtl_register_cfg_hooks ();
> > + else
> > +rtl_register_cfg_hooks ();
>
> Do we expect to change this? I'd just get rid of the if (0), at least
> for now.
Patch 9 adds some logic for switching the hooks as we go through the
various passes, so that "startwith" works properly, but here we just
need it to reflect the state of the hooks coming out of "expand", so
I've changed it to:
/* We start in cfgrtl mode, rather than cfglayout mode. */
rtl_register_cfg_hooks ();
> > +/* cgraph_node::add_new_function does additional processing
> > + based on symtab->state. We need to avoid it attempting to
> > gimplify
> > + things. Temporarily putting it in the PARSING state
> > appears to
> > + achieve this. */
> > +enum symtab_state old_state = symtab->state;
> > +symtab->state = PARSING;
> > +cgraph_node::add_new_function (fndecl, true /*lowered*/);
> > +/* Reset the state. */
> > +symtab->state = old_state;
> > + }
>
> Does it do anything beside call finalize_function in that state? If
> that's all you need, just call it directly.
There's some logic after the switch on state for calling
lang_hooks.eh_personality and setting DECL_FUNCTION_PERSONALITY, but I
think if we want to support that, we'd add it as additional dumped
data.
I've updated things to call finalize_function directly, removing the
messing about with the symtab state.
> > +
> > + /* Parse DECL_RTL. */
> > + {
> > +require_char_ws ('(');
> > +require_word_ws ("DECL_RTL");
> > +DECL_WRTL_CHECK (t_param)->decl_with_rtl.rtl = parse_rtx ();
>
Re: [PATCH 8/9] Introduce class function_reader (v4)
On 11/11/2016 10:15 PM, David Malcolm wrote:
#include "gt-aarch64.h"
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 6c608e0..0dda786 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
I think we should separate out the target specific tests so as to give
port maintainers a chance to comment on them separately.
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 50cd388..179a91f 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -1371,6 +1371,19 @@ maybe_set_first_label_num (rtx_code_label *x)
if (CODE_LABEL_NUMBER (x) < first_label_num)
first_label_num = CODE_LABEL_NUMBER (x);
}
+
+/* For use by the RTL function loader, when mingling with normal
+ functions.
Not sure what this means.
if (str == 0)
- fputs (" \"\"", m_outfile);
+ fputs (" (nil)", m_outfile);
else
fprintf (m_outfile, " (\"%s\")", str);
m_sawclose = 1;
What does this affect?
/* Global singleton; constrast with md_reader_ptr above. */
diff --git a/gcc/read-rtl-function.c b/gcc/read-rtl-function.c
new file mode 100644
index 000..ff6c808
--- /dev/null
+++ b/gcc/read-rtl-function.c
@@ -0,0 +1,2124 @@
+/* read-rtl-function.c - Reader for RTL function dumps
+ Copyright (C) 2016 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+#include
Please double-check all these includes whether they are necessary.
+
+/* Fix up a NOTE_INSN_BASIC_BLOCK based on an integer block ID. */
+
+void
+fixup_note_insn_basic_block::apply (function_reader */*reader*/) const
Lose the /*reader*/, probably.
+
+/* Implementation of rtx_reader::handle_unknown_directive.
+
+ Require a top-level "function" elements, as emitted by
+ print_rtx_function, and parse it. */
"element"?
+void
+function_reader::create_function ()
+{
+ /* Currently we assume cfgrtl mode, rather than cfglayout mode. */
+ if (0)
+cfg_layout_rtl_register_cfg_hooks ();
+ else
+rtl_register_cfg_hooks ();
Do we expect to change this? I'd just get rid of the if (0), at least
for now.
+/* cgraph_node::add_new_function does additional processing
+ based on symtab->state. We need to avoid it attempting to gimplify
+ things. Temporarily putting it in the PARSING state appears to
+ achieve this. */
+enum symtab_state old_state = symtab->state;
+symtab->state = PARSING;
+cgraph_node::add_new_function (fndecl, true /*lowered*/);
+/* Reset the state. */
+symtab->state = old_state;
+ }
Does it do anything beside call finalize_function in that state? If
that's all you need, just call it directly.
+
+ /* Parse DECL_RTL. */
+ {
+require_char_ws ('(');
+require_word_ws ("DECL_RTL");
+DECL_WRTL_CHECK (t_param)->decl_with_rtl.rtl = parse_rtx ();
+require_char_ws (')');
+ }
Spurious { } blocks.
+ if (0)
+fprintf (stderr, "parse_edge: %i flags 0x%x \n",
+other_bb_idx, flags);
Remove this.
+ /* For now, only process the (edge-from) to this BB, and (edge-to)
+ that go to the exit block; we don't yet verify that the edge-from
+ and edge-to directives are consistent. */
That's probably worth a FIXME.
+ if (rtx_code_label *label = dyn_cast (insn))
+maybe_set_max_label_num (label);
I keep forgetting why dyn_cast instead of as_a?
+case 'e':
+ {
+ if (idx == 7 && CALL_P (return_rtx))
+ {
+ m_in_call_function_usage = true;
+ return rtx_reader::read_rtx_operand (return_rtx, idx);
+ m_in_call_function_usage = false;
+ }
+ else
+ return rtx_reader::read_rtx_operand (return_rtx, idx);
+ }
+ break;
Unnecessary { } blocks in several places.
+
+case 'w':
+ {
+ if (!is_compact ())
+ {
+ /* Strip away the redundant hex dump of the value. */
+ require_char_ws ('[');
+ read_name (&name);
+ require_char_ws (']');
+ }
+ }
+ break;
Here too.
+
+/* Special-cased handling of codes 'i' and 'n' for reading function
+ dumps. */
+
+void
+function_reader::read_rtx_operand_i_or_n (rtx return_rtx, int idx,
+ char format_char)
Document arguments (everywhere). I think return_rtx (throughout these
functions) is a poor name that can cause confusion because it seems to
imply a (return).
+
+ /* Possibly wrote:
+print_node_brief (outfile, "", SYMBOL_REF_DECL (in_rtx),
+ dump_flags); */
???
+ /* Skip the content for now. */
Does this relate to the above? Please clarify the comments.
+ case CODE_LABEL:
+ {
+ /* Assume that LABEL_NUSES was not dumped. */
+ /* TODO: parse LABEL_KIND. */
Unnecessary { }.
+ if (0 == strcmp (desc, ""))
+{
+ re
Re: [PATCH 8/9] Introduce class function_reader (v4)
On Wed, 2016-11-23 at 21:15 +0100, Bernd Schmidt wrote:
> On 11/11/2016 10:15 PM, David Malcolm wrote:
>
> > +static void
> > +aarch64_test_loading_full_dump ()
> > +{
> > + rtl_dump_test t (SELFTEST_LOCATION, locate_file ("aarch64/times
> > -two.rtl"));
> > +
> > + ASSERT_STREQ ("times_two", IDENTIFIER_POINTER (DECL_NAME (cfun
> > ->decl)));
> > +
> > + rtx_insn *insn_1 = get_insn_by_uid (1);
> > + ASSERT_EQ (NOTE, GET_CODE (insn_1));
> > +
> > + rtx_insn *insn_15 = get_insn_by_uid (15);
> > + ASSERT_EQ (INSN, GET_CODE (insn_15));
> > + ASSERT_EQ (USE, GET_CODE (PATTERN (insn_15)));
> > +
> > + /* Verify crtl->return_rtx. */
> > + ASSERT_EQ (REG, GET_CODE (crtl->return_rtx));
> > + ASSERT_EQ (0, REGNO (crtl->return_rtx));
> > + ASSERT_EQ (SImode, GET_MODE (crtl->return_rtx));
> > +}
>
> The problem I'm having with this patch, and why I've not really
> commented on the latter parts of the series, is that I was hoping the
> tests would be more self-contained.
The tests in this patch (8/9) are more about verifying that the loader
is sane: they load a dump, and then assert various properties of it, to
ensure that we're loading things correctly. I moved the example dumps
out of the .c files (using locate_file) to avoid having to embed them
in the .c file and escape things (which people objected to in an
earlier version of the kit).
> For transformations I had mentioned
> the idea of having both before and after dumps.
>
> Having a few test files with C code to verify them is probably not a
> big
> deal, but I wonder whether this is going to be the norm rather than
> the
> exception, and whether there are better ways of doing it.
These will be the exception; they're really just about verifying that
the loader is working correctly.
In a previous version of the kit there were indeed selftests that
loaded a dump and attempted to run parts of a pass on it. I've deleted
all of those style of selftest.
> Maybe there
> needs to be a way of annotating the input RTL to tell the compiler
> what
> kinds of tests to run on it.
The "real" tests are in patch 9 of 9, and use DejaGnu directives
(scanning dumps, and "dg-do run" to actually run the generated code).
Re: [PATCH 8/9] Introduce class function_reader (v4)
On 11/11/2016 10:15 PM, David Malcolm wrote:
+static void
+aarch64_test_loading_full_dump ()
+{
+ rtl_dump_test t (SELFTEST_LOCATION, locate_file ("aarch64/times-two.rtl"));
+
+ ASSERT_STREQ ("times_two", IDENTIFIER_POINTER (DECL_NAME (cfun->decl)));
+
+ rtx_insn *insn_1 = get_insn_by_uid (1);
+ ASSERT_EQ (NOTE, GET_CODE (insn_1));
+
+ rtx_insn *insn_15 = get_insn_by_uid (15);
+ ASSERT_EQ (INSN, GET_CODE (insn_15));
+ ASSERT_EQ (USE, GET_CODE (PATTERN (insn_15)));
+
+ /* Verify crtl->return_rtx. */
+ ASSERT_EQ (REG, GET_CODE (crtl->return_rtx));
+ ASSERT_EQ (0, REGNO (crtl->return_rtx));
+ ASSERT_EQ (SImode, GET_MODE (crtl->return_rtx));
+}
The problem I'm having with this patch, and why I've not really
commented on the latter parts of the series, is that I was hoping the
tests would be more self-contained. For transformations I had mentioned
the idea of having both before and after dumps.
Having a few test files with C code to verify them is probably not a big
deal, but I wonder whether this is going to be the norm rather than the
exception, and whether there are better ways of doing it. Maybe there
needs to be a way of annotating the input RTL to tell the compiler what
kinds of tests to run on it.
Bernd
