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 0000000..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 <mpfr.h>
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 <rtx_code_label *> (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, "<retval>")) + { + return DECL_RESULT (fndecl); + } + + /* Search within function parms. */ + for (tree arg = DECL_ARGUMENTS (fndecl); arg; arg = TREE_CHAIN (arg)) + { + if (strcmp (desc, IDENTIFIER_POINTER (DECL_NAME (arg))) == 0) + return arg; + }
No { } around single statements, both cases.
+ /* Not found? Create it. + This allows mimicing of real data but avoids having to specify
"mimicking".
+rtx +function_reader::consolidate_singletons (rtx x) +{ + if (!x) + return x; + + switch (GET_CODE (x))
Formatting.
+ { + /* FIXME: do we need to check for VOIDmode for these? */
Don't see why we would.
+ case CONST_INT: + return gen_rtx_CONST_INT (GET_MODE (x), INTVAL (x));
Ugh, really? Can't this be done earlier?
+ + add_fixup_source_location (loc, insn, operand_idx, + filename, atoi(name.string));
Space before open paren.
+ /* Verify lookup of hard registers. */ +#ifdef GCC_AARCH64_H + ASSERT_EQ (0, lookup_reg_by_dump_name ("x0")); + ASSERT_EQ (1, lookup_reg_by_dump_name ("x1")); +#endif +#ifdef I386_OPTS_H + ASSERT_EQ (0, lookup_reg_by_dump_name ("ax")); + ASSERT_EQ (1, lookup_reg_by_dump_name ("dx")); +#endif
Same as before, please don't do this here.
+/* Verify that the src and dest indices and flags of edge E are as + expected, using LOC as the effective location when reporting + failures. */
Again, please document all args (everywhere).
+ +static void +test_loading_dump_fragment_1 () +{ + // TODO: filter on target? + rtl_dump_test t (SELFTEST_LOCATION, locate_file ("asr_div1.rtl"));
This is in a generic file, isn't it? Yeah, I don't think we want to load any RTL from here. Split out such selftests into a separate patch so we can figure out what to do with them.
+ + 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)?
+ +/* An optional policy class for class function_reader. */ + +struct function_reader_policy +{ + function_reader_policy () + { + } +}; + +extern bool read_rtl_function_body (int argc, const char **argv, + bool (*parse_opt) (const char *), + function_reader_policy *policy);
The policy seems completely unused, please remove. Also, can't this single declaration go into an existing header?
+ /* Handle insn codes in compact dumps. In such dumps, the code of insns + is prefixed with "c", giving "cinsn", "cnote" etc, and CODE_LABEL is + special-cased as "clabel". */ + if (name[0] == 'c') + { + for (i = 0; i < NUM_RTX_CODE; i++) + if (strcmp (GET_RTX_NAME (i), name + 1) == 0) + return i;
I'd rather check specifically for the ones we expect, to avoid surprises.
+#ifdef GENERATOR_FILE
There's a lot of this. Is there a clean split where stuff could be moved into a separate file instead?
@@ -1103,13 +1233,39 @@ rtx_reader::read_rtx_code (const char *code_name) rtx value; /* Value of this node. */ }; + one_time_initialization ();
Isn't there a better place to call this?
+ + /* Use the format_ptr to parse the various operands of this rtx. + read_rtx_operand is a vfunc, allowing the parser to vary between + parsing .md files and parsing .rtl dumps; the function_reader subclass + overrides the defaults when loading RTL dumps, to handle the + various extra attributes emitted by print_rtx. */
Not sure that documentation is really necessary at this point. If someone is looking for the definition of read_rtx_operand they'll figure out it's a virtual function anyway.
for (int idx = 0; format_ptr[idx] != 0; idx++) - read_rtx_operand (return_rtx, idx); + return_rtx = read_rtx_operand (return_rtx, idx); + + /* Call a vfunc to handle the various additional information that + print-rtl.c can write after the regular fields; does nothing when + parsing .md files. */ + handle_any_trailing_information (return_rtx);
Same here really.
+ + /* "stringbuf" was allocated within string_obstack and thus has + the its lifetime restricted to that of the rtx_reader. This is + OK for the generator programs, but for non-generator programs, + XSTR and XTMPL fields are meant to be allocated in the GC-managed + heap. Hence we need to allocate a copy in the GC-managed heap + for the non-generator case. */ + const char *string_ptr = stringbuf; +#ifndef GENERATOR_FILE + string_ptr = ggc_strdup (stringbuf); +#endif /* #ifndef GENERATOR_FILE */
Encapsulate in a virtual finalize_string perhaps? Bernd