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

Reply via email to