[PATCH] print_rtx_function: integrate dumping of the CFG into the insn chain

[David Malcolm <dmalcolm at redhat dot com>]

On Fri, 2016-10-07 at 15:58 +0200, Bernd Schmidt wrote:
> On 10/07/2016 03:26 PM, David Malcolm wrote:
> > 
> > We could simply print the INSN_UID for CODE_LABELs; something like
> > this
> > (see the "(code_label 16" below):
> 
> I think that should work.
> 
> > You appear to have trimmed the idea of enclosing the insns with
> > (basic-block) directives without commenting on it.  Did you like
> > this
> > idea?
> 
> Sorry - I appear to have completely missed it.
> 
> > It would make the above look like:
> > 
> >   (basic-block 2
> >     ;; insns snipped
> >     (jump_insn (set (pc)
> >             (if_then_else (ge (reg:CCGC 17 flags)
> >                     (const_int 0 [0]))
> >                 (label_ref 16)
> >                 (pc))) "test.c":3
> >    -> 16)
> >   ) ;; basic-block 2
> >   (basic-block 4
> >     (note [bb 4] NOTE_INSN_BASIC_BLOCK)
> >     ;; insns snipped
> >     (jump_insn (set (pc) (label_ref 20)) "test.c":4
> >      -> 20)
> >   ) ;; basic-block 4
> >   (barrier)
> >   (basic-block 5
> >     (code_label 16 [1 uses])
> >     (note [bb 5] NOTE_INSN_BASIC_BLOCK)
> >     ;; etc
> >   ) ;; basic-block 5
> > 
> > Note how the above format expresses clearly that:
> > * the (barrier) is part of the insn chain, but not in a basic
> > block, and
> > * some insns can happen in a basic block
> 
> That looks really nice IMO. Except maybe drop the "-> 16" bit for the
> jump_insn (that's the JUMP_LABEL, isn't it?)
> 
> > Taking this idea further: if we have (basic-block) directives
> > integrated into the insn-chain like this, we could express the CFG
> > by
> > adding (edge) directives. Here's a suggestion for doing it with
> > (edge-from) and (edge-to) directives, expressing the predecessor
> > and
> > successor edges in the CFG, along with :
> 
> That also looks reasonable. Probably a small but maybe not a huge
> improvement over the other syntax. Having both from and to edges
> seems
> redundant but might help readability. The reader should check
> consistency in that case.
> 
> > Should we spell "0" and "1" as "entry" and "exit" when
> > parsing/dumping
> > basic block indices? e.g.:
> > 
> >   (basic-block 2
> >     (edge-from entry)
> 
> If that can be done it would be an improvement.

The following patch implements the above idea, integrating (block)
directives into the (insn-chain) which wrap those rtx_insn * that
have a basic block.

It doesn't yet suppress printing the various INSN_UID fields, along
with the other ideas in this thread; I plan to do these as followups.

> > > I think maybe you want a separate compact form of insns and notes
> > > (cinsn/cnote maybe), with a flag selecting which gets written out
> > > in
> > > the
> > > dumper. The reader could then read them in like any other rtx
> > > code,
> > > and
> > > there'd be a postprocessing stage to reconstruct the insn chain.
> > 
> > By this separate compact form, do you mean the form we've been
> > discussing above, with no INSN_UID/PREV/NEXT, etc?  Or something
> > else?
> 
> Yes, the form we're discussing, except instead of (insn ...) you'd
> have
> (cinsn ...), which I assume would make it easier for the reader and
> less
> ambiguous overall.
> 
> > As for "cinsn", "cnote", how about just "insn" and "note", and
> > having
> > the compactness be expressed at the top of the dump e.g. implicitly
> > by
> > the presence of a "(function" directive.  Could even have a format
> > version specifier in the function directive, to give us some future
> > -proofing e.g.
> >   (function (format 20161007)
> > or somesuch.
> 
> Having it implicit should also be ok - I have no strong preference
> really. I'm not sure we want versioning - better to have an automatic
> conversion if we ever feel we need to change the format.
> 
> > Do you want to want to try hand-edited a test case, using some of
> > the
> > format ideas we've been discussing?  That might suggest further
> > improvements to the format.
> 
> We'll definitely want to have a look at one or two. Also, we ought to
> try to set up situations we haven't discussed: ADDR_VECs (in light of
> the basic block dumping) and ASMs maybe. I'm probably forgetting
> some.
> 
> One other thing in terms of format is the printout of CONST_INT - I
> think it should suffice to have either decimal, or hex, but not
> really
> both. The reader should maybe accept either.
> 
> I think all hosts have 64-bit HWI these days, so CONST_INT ought to
> always stay valid through a roundtrip.
> 
> I may have missed it, but is there any kind of provision yet for
> providing an "after" dump for what is expected after a pass is run?
> Might be worth thinking about whether the reader could have a mode
> where
> it matches internal RTL against an input.
> 
> > OK.  If so, do we need to print the regno for hard registers?  Or
> > should we just print the name for those, for consistency with
> > virtual
> > regs?  (i.e. have it be controlled by the same flag?)
> 
> Just the name should work, leaving only pseudos with numbers - that
> ought to be reasonable. In the reader, when encountering a reg with a
> number, just add FIRST_PSEUDO_REGISTER, and you should end up with
> something that's consistent. Or maybe even dump the expected
> FIRST_PSEUDO_REGISTER, and adjust for it in case the one we have at
> run-time differs.
> 
> > > Does the C one have an advantage in terms of meaningful decls in
> > > MEM_ATTRs/SYMBOL_REFs?
> > 
> > Probably.
> 
> I think that might be the more promising path then.
> 
> 
> Bernd

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu (on top
of this approved patch to add selftest::read_file:
  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00476.html
of which Jeff said "Seems reasonable.  Install when you have a need.":
  https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01095.html
which this patch makes selftest::test_expansion_to_rtl make use of.

OK for trunk?

gcc/ChangeLog:
	* function-tests.c: Include "print-rtl.h".
	(selftest::test_expansion_to_rtl): Call print_rtx_function on the
	function, and verify what is dumped.
	* print-rtl-function.c (print_edge): New function.
	(begin_any_block): New function.
	(end_any_block): New function.
	(can_have_basic_block_p): New function.
	(print_rtx_function): Track the basic blocks of insns in the
	chain, wrapping those that are within blocks within "(block)"
	directives.  Remove the "(cfg)" directive.
---
 gcc/function-tests.c     |  23 +++++++
 gcc/print-rtl-function.c | 156 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 140 insertions(+), 39 deletions(-)

diff --git a/gcc/function-tests.c b/gcc/function-tests.c
index 4152cd3..04df8d8 100644
--- a/gcc/function-tests.c
+++ b/gcc/function-tests.c
@@ -78,6 +78,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "ipa-ref.h"
 #include "cgraph.h"
 #include "selftest.h"
+#include "print-rtl.h"
 
 #if CHECKING_P
 
@@ -643,6 +644,28 @@ test_expansion_to_rtl ()
 
   /* ...etc; any further checks are likely to over-specify things
      and run us into target dependencies.  */
+
+  /* Verify that print_rtl_function is sane.  */
+  named_temp_file tmp_out (".rtl");
+  FILE *outfile = fopen (tmp_out.get_filename (), "w");
+  print_rtx_function (outfile, fun);
+  fclose (outfile);
+
+  char *dump = read_file (SELFTEST_LOCATION,
+			  tmp_out.get_filename ());
+  ASSERT_STR_CONTAINS (dump, "(function \"test_fn\"\n");
+  ASSERT_STR_CONTAINS (dump, "  (insn-chain\n");
+  ASSERT_STR_CONTAINS (dump, "    (block 2\n");
+  ASSERT_STR_CONTAINS (dump, "      (edge-from entry (flags \"FALLTHRU\"))\n");
+  ASSERT_STR_CONTAINS (dump, "      (insn "); /* ...etc.  */
+  ASSERT_STR_CONTAINS (dump, "      (edge-to exit (flags \"FALLTHRU\"))\n");
+  ASSERT_STR_CONTAINS (dump, "    ) ;; block 2\n");
+  ASSERT_STR_CONTAINS (dump, "  ) ;; insn-chain\n");
+  ASSERT_STR_CONTAINS (dump, "  (crtl\n");
+  ASSERT_STR_CONTAINS (dump, "  ) ;; crtl\n");
+  ASSERT_STR_CONTAINS (dump, ") ;; function \"test_fn\"\n");
+
+  free (dump);
 }
 
 /* Run all of the selftests within this file.  */
diff --git a/gcc/print-rtl-function.c b/gcc/print-rtl-function.c
index c4b99c0..ba883fa 100644
--- a/gcc/print-rtl-function.c
+++ b/gcc/print-rtl-function.c
@@ -33,14 +33,102 @@ along with GCC; see the file COPYING3.  If not see
 #include "langhooks.h"
 #include "emit-rtl.h"
 
+/* Print an "(edge-from)" or "(edge-to)" directive describing E
+   to OUTFILE.  */
+
+static void
+print_edge (FILE *outfile, edge e, bool from)
+{
+  fprintf (outfile, "      (%s ", from ? "edge-from" : "edge-to");
+  basic_block bb = from ? e->src : e->dest;
+  gcc_assert (bb);
+  switch (bb->index)
+    {
+    case ENTRY_BLOCK:
+      fprintf (outfile, "entry");
+      break;
+    case EXIT_BLOCK:
+      fprintf (outfile, "exit");
+      break;
+    default:
+      fprintf (outfile, "%i", bb->index);
+      break;
+    }
+
+  /* Express edge flags as a string with " | " separator.
+     e.g. (flags "FALLTHRU | DFS_BACK").  */
+  fprintf (outfile, " (flags \"");
+  bool seen_flag = false;
+#define DEF_EDGE_FLAG(NAME,IDX) \
+  do {						\
+    if (e->flags & EDGE_##NAME)			\
+      {						\
+	if (seen_flag)				\
+	  fprintf (outfile, " | ");		\
+	fprintf (outfile, "%s", (#NAME));	\
+	seen_flag = true;			\
+      }						\
+  } while (0);
+#include "cfg-flags.def"
+#undef DEF_EDGE_FLAG
+
+  fprintf (outfile, "\"))\n");
+}
+
+/* If BB is non-NULL, print the start of a "(block)" directive for it
+   to OUTFILE, otherwise do nothing.  */
+
+static void
+begin_any_block (FILE *outfile, basic_block bb)
+{
+  if (!bb)
+    return;
+
+  edge e;
+  edge_iterator ei;
+
+  fprintf (outfile, "    (block %i\n", bb->index);
+  FOR_EACH_EDGE (e, ei, bb->preds)
+    print_edge (outfile, e, true);
+}
+
+/* If BB is non-NULL, print the end of a "(block)" directive for it
+   to OUTFILE, otherwise do nothing.  */
+
+static void
+end_any_block (FILE *outfile, basic_block bb)
+{
+  if (!bb)
+    return;
+
+  edge e;
+  edge_iterator ei;
+
+  FOR_EACH_EDGE (e, ei, bb->succs)
+    print_edge (outfile, e, false);
+  fprintf (outfile, "    ) ;; block %i\n", bb->index);
+}
+
+/* Determine if INSN is of a kind that can have a basic block.  */
+
+static bool
+can_have_basic_block_p (const rtx_insn *insn)
+{
+  return GET_RTX_FORMAT (GET_CODE (insn))[2] == 'B';
+}
+
 /* Write FN to OUTFILE in a form suitable for parsing, with indentation
-   and comments to make the structure easy for a human to grok.
+   and comments to make the structure easy for a human to grok.  Track
+   the basic blocks of insns in the chain, wrapping those that are within
+   blocks within "(block)" directives.
 
    Example output:
 
-     (function "times_two"
-       (insn-chain
-	 (note 1 0 4 (nil) NOTE_INSN_DELETED)
+   (function "times_two"
+     (insn-chain
+       (note 1 0 4 (nil) NOTE_INSN_DELETED)
+       (block 2
+	 (edge-from entry (flags "FALLTHRU"))
 	 (note 4 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
 	 (insn 2 4 3 2 (set (mem/c:SI (plus:DI (reg/f:DI 82 virtual-stack-vars)
 			     (const_int -4 [0xfffffffffffffffc])) [1 i+0 S4 A32])
@@ -69,23 +157,15 @@ along with GCC; see the file COPYING3.  If not see
 		  (nil))
 	 (insn 15 14 0 2 (use (reg/i:SI 0 ax)) t.c:4 -1
 		  (nil))
-       ) ;; insn-chain
-       (cfg
-	 (bb 0
-	   (edge 0 2 (flags 0x1))
-	 ) ;; bb
-	 (bb 2
-	   (edge 2 1 (flags 0x1))
-	 ) ;; bb
-	 (bb 1
-	 ) ;; bb
-       ) ;; cfg
-       (crtl
-	 (return_rtx
-	   (reg/i:SI 0 ax)
-	 ) ;; return_rtx
-       ) ;; crtl
-     ) ;; function "times_two"
+	 (edge-to exit (flags "FALLTHRU"))
+       ) ;; block 2
+     ) ;; insn-chain
+     (crtl
+       (return_rtx
+	  (reg/i:SI 0 ax)
+       ) ;; return_rtx
+     ) ;; crtl
+   ) ;; function "times_two"
 */
 
 DEBUG_FUNCTION void
@@ -99,27 +179,25 @@ print_rtx_function (FILE *outfile, function *fn)
 
   /* The instruction chain.  */
   fprintf (outfile, "  (insn-chain\n");
+  basic_block curr_bb = NULL;
   for (rtx_insn *insn = get_insns (); insn; insn = NEXT_INSN (insn))
-    print_rtl_single_with_indent (outfile, insn, 4);
+    {
+      basic_block insn_bb;
+      if (can_have_basic_block_p (insn))
+	insn_bb = BLOCK_FOR_INSN (insn);
+      else
+	insn_bb = NULL;
+      if (curr_bb != insn_bb)
+	{
+	  end_any_block (outfile, curr_bb);
+	  curr_bb = insn_bb;
+	  begin_any_block (outfile, curr_bb);
+	}
+      print_rtl_single_with_indent (outfile, insn, curr_bb ? 6 : 4);
+    }
+  end_any_block (outfile, curr_bb);
   fprintf (outfile, "  ) ;; insn-chain\n");
 
-  /* The CFG.  */
-  fprintf (outfile, "  (cfg\n");
-  {
-    basic_block bb;
-    FOR_ALL_BB_FN (bb, fn)
-      {
-	fprintf (outfile, "    (bb %i\n", bb->index);
-	edge e;
-	edge_iterator ei;
-	FOR_EACH_EDGE (e, ei, bb->succs)
-	  fprintf (outfile, "      (edge %i %i (flags 0x%x))\n",
-		   e->src->index, e->dest->index, e->flags);
-	fprintf (outfile, "    ) ;; bb\n");
-      }
-  }
-  fprintf (outfile, "  ) ;; cfg\n");
-
   /* Additional RTL state.  */
   fprintf (outfile, "  (crtl\n");
   fprintf (outfile, "    (return_rtx \n");
-- 
1.8.5.3