On Fri, 2016-12-02 at 15:41 +0100, Bernd Schmidt wrote:
> 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.
Thanks for the reviews so far.
> 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.
(nods)
> >
> > + /* Lookup param by name. */
> > + tree t_param = parse_mem_expr (name);
> > + // TODO: what if not found?
>
> Error out?
Currently, function_reader::parse_mem_expr can't fail; it creates a
VAR_DECL for anything it doesn't recognize.
But here, we should be looking specifically for a PARAM_DECL.
I've moved the find-param-by-name code out from
function_reader::parse_mem_expr into a new function, and call that
instead, and error out if it isn't found from that.
> > +/* 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.
Done.
> > + /* 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.
The two flag assignments don't seem to be needed; I think this is due
to adding:
if (node->native_rtl_p ())
node->force_output = 1;
to cgraph_node::finalize_function in patch 9.
Should I lose them (and 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.
Done. In doing so, I noticed that the args to read_rtl_function_body
were unnecessarily complicated, so I simplified it to just take a path.
I also made some simplifications to the various fixup_source_location
functions.
> > +
> > +/* Parse operand IDX of X, of code 'i' or 'n'.
>
> "... as specified by FORMAT_CHAR", presumably.
Done.
> > + 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.
This is involved; let's do this as a followup.
> > + /* 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.
Removed for now.
> > @@ -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.
Aha; that makes much more sense.
Done.
> > +
> > + /* 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 o
