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 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.
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 ();
> > + require_char_ws (')');
> > + }
>
> Spurious { } blocks.
Removed.
> > + if (0)
> > + fprintf (stderr, "parse_edge: %i flags 0x%x \n",
> > + other_bb_idx, flags);
>
> Remove this.
Removed.
> > + /* 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.
Added.
> > + 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?
as_a requires that the cast be valid; dyn_cast, determines if the cast
is valid, and returns NULL if it isn't valid, allowing uses within a
conditional like the one here.
> > + 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.
Fixed.
> > +
> > + 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.
Fixed.
> > +
> > +/* 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).
I've renamed it to "x" throughout. I've gone through and documented
arguments and return values throughout (I hope). In doing so, I found
extra_parsing_for_operand_code_0's return value to be redundant, so I
made it "void".
> > +
> > + /* Possibly wrote:
> > + print_node_brief (outfile, "", SYMBOL_REF_DECL (in_rtx),
> > + dump_flags); */
>
> ???
I've rewritten the comment to read:
/* If X had a non-NULL SYMBOL_REF_DECL,
rtx_writer::print_rtx_operand_code_0 would have dumped it
using print_node_brief.
Skip the content for now. */
Hopefully that's clearer.
> > + /* Skip the content for now. */
>
> Does this relate to the above? Please clarify the comments.
Indeed; I've merged it into the above comment.
>
> > + case CODE_LABEL:
> > + {
> > + /* Assume that LABEL_NUSES was not dumped. */
> > + /* TODO: parse LABEL_KIND. */
>
> Unnecessary { }.
Fixed.
> > + 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.
Fixed.
> > + /* Not found? Create it.
> > + This allows mimicing of real data but avoids having to
> > specify
>
> "mimicking".
Fixed.
> > +rtx
> > +function_reader::consolidate_singletons (rtx x)
> > +{
> > + if (!x)
> > + return x;
> > +
> > + switch (GET_CODE (x))
>
> Formatting.
Fixed.
> > + {
> > + /* FIXME: do we need to check for VOIDmode for these? */
>
> Don't see why we would.
Thanks; removed.
> > + case CONST_INT:
> > + return gen_rtx_CONST_INT (GET_MODE (x), INTVAL (x));
>
> Ugh, really? Can't this be done earlier?
rtx_reader::read_rtx_code reads the name "const_int" from the FILE *
turns it into a RTX_CODE, and allocates a fresh instance via:
rtx_alloc (code)
I guess we could special-case it at that point, but it would mean
special-casing reading the operands etc also.
By doing it here, we consolidate all of the singleton-consolidation
into one place.
> > +
> > + add_fixup_source_location (loc, insn, operand_idx,
> > + filename, atoi(name.string));
>
> Space before open paren.
Fixed.
> > + /* 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.
lookup_reg_by_dump_name is static within read-rtl-function.c. I could
expose it within e.g. rtl.h, and then do the selftests within a target
-specific selftest hook.
Alternatively, I could add the verification of loading hard-reg regnos
to the "loading a dump" selftests I've just split out into separate
patches.
Do you have a preference?
> > +/* 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).
Fixed.
> > +
> > +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.
Yes: this is a target-independent dump (I *think*; works with both
x86_64 and aarch64).
I've split out all of them into another patch.
> > +
> > + 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?
> > +
> > +/* 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.
Oops, yes; it's a vestige from an older version of pseudo handling.
Removed.
> Also, can't this
> single declaration go into an existing header?
Probably, but I'm not sure where.
I tried rtl.h, but patch 9 adds read_rtl_function_body_from_file_range.
This needs to be callable from the C frontend, and rtl.h is poisoned
within frontends. Hence the new header file.
> > + /* 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.
I've converted it to a table of known values.
> > +#ifdef GENERATOR_FILE
>
> There's a lot of this. Is there a clean split where stuff could be
> moved
> into a separate file instead?
Currently we have:
- read-md.c: base .md reading, with base-bones directive-handling
- read-rtl.c: adds the ability to handle hierarchical rtx
- read-rtl-function.c: (new in patch kit): adds the ability to read RTL
function dumps.
So maybe "read-rtl-md.c", for the things that relate to hierarchical
rtx, but which are .md-specific?
Or I could try to move the things to read-md.c, but not sure if that's
going to work.
> > @@ -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?
Maybe rename to "ensure_initialized"? This replaces the code:
- static bool initialized = false;
-
- /* Do one-time initialization. */
- if (!initialized)
- {
- initialize_iterators ();
- initialized = true;
- }
in rtx_reader::read_rtx, since now not all rtx-reading goes through
that path.
> > +
> > + /* 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.
Given that we rarely use vfuncs, is calling it out more acceptable?
Shall I just drop it, or word it as "Default implementation of..."?
> > 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.
Maybe reword to:
"Give subclasses a chance to handle any additional information that
may be present after the regular fields (e.g. in a function dump)"
or somesuch.
> > +
> > + /* "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?
Done.
I'm preparing updated patches (for the issues covered so far...)
Dave
