On 12/02/2016 03:00 AM, David Malcolm wrote:
Changed in v6:
- split out dump-reading selftests into followup patches
(target-independent, and target-specific)
- removes unneeded headers from read-rtl-function.c
- numerous other cleanups identified in review
Ok, starting to look very close to done.
It appears I accidentally made you change element->directive. It seems
like a good change but all I wanted to point out was a plural where I
would have expected a singular.
+ /* Lookup param by name. */
+ tree t_param = parse_mem_expr (name);
+ // TODO: what if not found?
Error out?
+/* Implementation of rtx_reader::handle_unknown_directive.
+
+ Require a top-level "function" directive, as emitted by
+ print_rtx_function, and parse it. */
Should document START_LOC and NAME.
+ /* Do we need this to force cgraphunit.c to output the function? */
Well, what happens if you don't do this? Seems reasonable anyway, so
maybe lose the comment.
+/* Parse rtx instructions by calling read_rtx_code, calling
+ set_first_insn and set_last_insn as appropriate, and
+ adding the insn to the insn chain.
+ Consume the trailing ')'. */
+
+rtx_insn *
+function_reader::parse_insn (file_location start_loc, const char *name)
Once again, please document args - make another pass over all the
functions to make sure.
+
+/* Parse operand IDX of X, of code 'i' or 'n'.
"... as specified by FORMAT_CHAR", presumably.
+ if (0 == strncmp (desc, "orig:", 5))
+ {
+ expect_original_regno = true;
+ desc_start += 5;
+ /* Skip to any whitespace following the integer. */
+ const char *space = strchr (desc_start, ' ');
+ if (space)
+ desc_start = space + 1;
+ }
+ /* Any remaining text may be the REG_EXPR. Alternatively we have
+ no REG_ATTRS, and instead we have ORIGINAL_REGNO. */
+ if (ISDIGIT (*desc_start))
+ {
+ /* Assume we have ORIGINAL_REGNO. */
+ ORIGINAL_REGNO (x) = atoi (desc_start);
+ }
This looks like an issue. You'll want to have ORIGINAL_REGNOs printed
and parsed like other regnos, i.e. with %number for pseudos. Can be done
as a followup if it's involved.
+ /* 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
Please just remove this for now; not important enough to break the rule
about target-specific bits in generic code.
@@ -1103,13 +1244,39 @@ rtx_reader::read_rtx_code (const char *code_name)
rtx value; /* Value of this node. */
};
+ one_time_initialization ();
I still don't understand why this isn't in the rtx_reader constructor?
Seems pointless to call this each time we want to parse an rtx.
+
+ /* 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. */
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);
I still think just drop the bulk of these comments. Virtual functions
aren't that arcane.
+ /* "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 = finalize_string (stringbuf);
Put the comment near the implementation that makes the copy (and maybe
one explaining why the no-op in the other implementation), not here.
Bernd
