[PATCH] Add a "compact" mode to print_rtx_function

[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.
> 
> > > 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.

This patch implements a "compact" mode for print_rtx_function,
implementing most of the ideas above.

Example of output can be seen here:
  https://dmalcolm.fedorapeople.org/gcc/2016-10-12/test-switch-compact.rtl
which can be contrasted with the non-compact output here:
  https://dmalcolm.fedorapeople.org/gcc/2016-10-12/test-switch-noncompact.rtl

It adds the "c" prefix to the insn names, so we get "cinsn", etc.  However,
it does lead to things like this:

   (ccode_label 56 8 "")

which gives me pause: would the "ccode" in "ccode_label" be confusing? (compared
to "ccmode").  An alternative might be to have a "compact-insn-chain" vs
"insn-chain" wrapper element, expressing that this is a compact dump.

Successfully bootstrapped on x86_64-pc-linux-gnu; regrtesting in progress.

OK for trunk if it passes?

I think the only remaining item from our discussion above is what to do
about the numbering of pseudos in the dumps (currently it just prints the regno
unmodified).

Other than that, is the resultant dump format good enough that I can start
rewriting the RTL frontend parser, or are there other changes you'd want?

gcc/ChangeLog:
	* function-tests.c (selftest::test_expansion_to_rtl): Add "true"
	for new "compact" param of print_rtx_function.  Check for "cinsn"
	rather than "insn".
	* print-rtl-function.c (flag_compact): New decl.
	(print_rtx_function): Add param "compact" and use it to set
	flag_compact, adding a description of the effect to the leading
	comment, and updating the example output.
	* print-rtl.c (flag_compact): New variable.
	(print_rtx_operand_code_0): Omit the JUMP_LABEL reference in compact
	mode.
	(print_rtx_operand_code_i): When printing source locations, wrap
	xloc.file in quotes.  Don't print INSN_CODEs in compact mode.
	(print_rtx_operand_code_r): Don't print regnos for hard regs and
	virtuals in compact mode.
	(print_rtx_operand_code_u): Don't print insn UIDs in compact mode,
	apart from in LABEL_REFs.
	(print_rtx_operand): In case 'w', don't print in hex in compact mode.
	Don't print basic block ids in compact mode.
	(print_rtx):  In compact mode, prefix the code of insns with "c",
	only print the INSN_UID of CODE_LABELs, and omit their LABEL_NUSES.
	* print-rtl.h (print_rtx_function): Add "compact" param.
---
 gcc/function-tests.c     |  4 +--
 gcc/print-rtl-function.c | 76 +++++++++++++++++++++++++++--------------------
 gcc/print-rtl.c          | 77 ++++++++++++++++++++++++++++++++++--------------
 gcc/print-rtl.h          |  2 +-
 4 files changed, 102 insertions(+), 57 deletions(-)

diff --git a/gcc/function-tests.c b/gcc/function-tests.c
index 049a07f9..b0c44cf 100644
--- a/gcc/function-tests.c
+++ b/gcc/function-tests.c
@@ -648,7 +648,7 @@ test_expansion_to_rtl ()
   /* 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);
+  print_rtx_function (outfile, fun, true);
   fclose (outfile);
 
   char *dump = read_file (SELFTEST_LOCATION, tmp_out.get_filename ());
@@ -656,7 +656,7 @@ test_expansion_to_rtl ()
   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, "      (cinsn "); /* ...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");
diff --git a/gcc/print-rtl-function.c b/gcc/print-rtl-function.c
index 4f9b4ef..90a0ff7 100644
--- a/gcc/print-rtl-function.c
+++ b/gcc/print-rtl-function.c
@@ -33,6 +33,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "langhooks.h"
 #include "emit-rtl.h"
 
+extern bool flag_compact;
+
 /* Print an "(edge-from)" or "(edge-to)" directive describing E
    to OUTFILE.  */
 
@@ -126,55 +128,63 @@ can_have_basic_block_p (const rtx_insn *insn)
    the basic blocks of insns in the chain, wrapping those that are within
    blocks within "(block)" directives.
 
-   Example output:
+   If COMPACT, then instructions are printed in a compact form:
+   - INSN_UIDs are omitted, except for jumps and CODE_LABELs,
+   - INSN_CODEs are omitted,
+   - register numbers are omitted for hard and virtual regs
+   - insn names are prefixed with "c" (e.g. "cinsn", "cnote", etc)
+
+   Example output (with COMPACT==true):
 
    (function "times_two"
      (insn-chain
-       (note 1 0 4 (nil) NOTE_INSN_DELETED)
+       (cnote 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])
-		     (reg:SI 5 di [ i ])) t.c:2 -1
-		  (nil))
-	 (note 3 2 6 2 NOTE_INSN_FUNCTION_BEG)
-	 (insn 6 3 7 2 (set (reg:SI 89)
-		     (mem/c:SI (plus:DI (reg/f:DI 82 virtual-stack-vars)
-			     (const_int -4 [0xfffffffffffffffc])) [1 i+0 S4 A32])) t.c:3 -1
-		  (nil))
-	 (insn 7 6 10 2 (parallel [
-			 (set (reg:SI 87 [ _2 ])
-			     (ashift:SI (reg:SI 89)
-				 (const_int 1 [0x1])))
-			 (clobber (reg:CC 17 flags))
-		     ]) t.c:3 -1
-		  (expr_list:REG_EQUAL (ashift:SI (mem/c:SI (plus:DI (reg/f:DI 82 virtual-stack-vars)
-				 (const_int -4 [0xfffffffffffffffc])) [1 i+0 S4 A32])
-			 (const_int 1 [0x1]))
-		     (nil)))
-	 (insn 10 7 14 2 (set (reg:SI 88 [ <retval> ])
-		     (reg:SI 87 [ _2 ])) t.c:3 -1
-		  (nil))
-	 (insn 14 10 15 2 (set (reg/i:SI 0 ax)
-		     (reg:SI 88 [ <retval> ])) t.c:4 -1
-		  (nil))
-	 (insn 15 14 0 2 (use (reg/i:SI 0 ax)) t.c:4 -1
-		  (nil))
+	 (cnote [bb 2] NOTE_INSN_BASIC_BLOCK)
+	 (cinsn (set (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars)
+			       (const_int -4)) [1 i+0 S4 A32])
+		       (reg:SI di [ i ])) "t.c":2
+		   (nil))
+	 (cnote NOTE_INSN_FUNCTION_BEG)
+	 (cinsn (set (reg:SI 89)
+		       (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars)
+			       (const_int -4)) [1 i+0 S4 A32])) "t.c":3
+		   (nil))
+	 (cinsn (parallel [
+			   (set (reg:SI 87 [ _2 ])
+			       (ashift:SI (reg:SI 89)
+				   (const_int 1)))
+			   (clobber (reg:CC flags))
+		       ]) "t.c":3
+		   (expr_list:REG_EQUAL (ashift:SI (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars)
+				   (const_int -4)) [1 i+0 S4 A32])
+			   (const_int 1))
+		       (nil)))
+	 (cinsn (set (reg:SI 88 [ <retval> ])
+		       (reg:SI 87 [ _2 ])) "t.c":3
+		   (nil))
+	 (cinsn (set (reg/i:SI ax)
+		       (reg:SI 88 [ <retval> ])) "t.c":4
+		   (nil))
+	 (cinsn (use (reg/i:SI ax)) "t.c":4
+		   (nil))
 	 (edge-to exit (flags "FALLTHRU"))
        ) ;; block 2
      ) ;; insn-chain
      (crtl
        (return_rtx
-	  (reg/i:SI 0 ax)
+	 (reg/i:SI ax)
        ) ;; return_rtx
      ) ;; crtl
    ) ;; function "times_two"
 */
 
 DEBUG_FUNCTION void
-print_rtx_function (FILE *outfile, function *fn)
+print_rtx_function (FILE *outfile, function *fn, bool compact)
 {
+  flag_compact = compact;
+
   tree fdecl = fn->decl;
 
   const char *dname = lang_hooks.decl_printable_name (fdecl, 2);
@@ -210,4 +220,6 @@ print_rtx_function (FILE *outfile, function *fn)
   fprintf (outfile, "  ) ;; crtl\n");
 
   fprintf (outfile, ") ;; function \"%s\"\n", dname);
+
+  flag_compact = false;
 }
diff --git a/gcc/print-rtl.c b/gcc/print-rtl.c
index 29e8ee2..88e9f49 100644
--- a/gcc/print-rtl.c
+++ b/gcc/print-rtl.c
@@ -60,6 +60,13 @@ static int indent;
 
 static bool in_call_function_usage;
 
+/* If true, use compact dump format:
+   - INSN_UIDs are omitted, except for jumps and CODE_LABELs,
+   - INSN_CODEs are omitted,
+   - register numbers are omitted for hard and virtual regs
+   - insn names are prefixed with "c" (e.g. "cinsn", "cnote", etc).  */
+bool flag_compact;
+
 static void print_rtx (const_rtx);
 
 /* String printed at beginning of each RTL when it is dumped.
@@ -176,7 +183,8 @@ print_rtx_operand_code_0 (const_rtx in_rtx ATTRIBUTE_UNUSED,
 	  break;
 	}
     }
-  else if (idx == 7 && JUMP_P (in_rtx) && JUMP_LABEL (in_rtx) != NULL)
+  else if (idx == 7 && JUMP_P (in_rtx) && JUMP_LABEL (in_rtx) != NULL
+	   && !flag_compact)
     {
       /* Output the JUMP_LABEL reference.  */
       fprintf (outfile, "\n%s%*s -> ", print_rtx_head, indent * 2, "");
@@ -284,7 +292,7 @@ print_rtx_operand_code_i (const_rtx in_rtx, int idx)
       if (INSN_HAS_LOCATION (in_insn))
 	{
 	  expanded_location xloc = insn_location (in_insn);
-	  fprintf (outfile, " %s:%i", xloc.file, xloc.line);
+	  fprintf (outfile, " \"%s\":%i", xloc.file, xloc.line);
 	}
 #endif
     }
@@ -335,6 +343,13 @@ print_rtx_operand_code_i (const_rtx in_rtx, int idx)
       const char *name;
       int is_insn = INSN_P (in_rtx);
 
+      /* Don't print INSN_CODEs in compact mode.  */
+      if (flag_compact && is_insn && &INSN_CODE (in_rtx) == &XINT (in_rtx, idx))
+	{
+	  sawclose = 0;
+	  return;
+	}
+
       if (flag_dump_unnumbered
 	  && (is_insn || NOTE_P (in_rtx)))
 	fputc ('#', outfile);
@@ -358,26 +373,28 @@ print_rtx_operand_code_r (const_rtx in_rtx)
   unsigned int regno = REGNO (in_rtx);
 
 #ifndef GENERATOR_FILE
+  /* For hard registers and virtuals, always print the
+     regno, except in compact mode.  */
+  if (regno <= LAST_VIRTUAL_REGISTER && !flag_compact)
+    fprintf (outfile, " %d", regno);
   if (regno < FIRST_PSEUDO_REGISTER)
-    fprintf (outfile, " %d %s", regno, reg_names[regno]);
+    fprintf (outfile, " %s", reg_names[regno]);
   else if (regno <= LAST_VIRTUAL_REGISTER)
     {
       if (regno == VIRTUAL_INCOMING_ARGS_REGNUM)
-	fprintf (outfile, " %d virtual-incoming-args", regno);
+	fprintf (outfile, " virtual-incoming-args");
       else if (regno == VIRTUAL_STACK_VARS_REGNUM)
-	fprintf (outfile, " %d virtual-stack-vars", regno);
+	fprintf (outfile, " virtual-stack-vars");
       else if (regno == VIRTUAL_STACK_DYNAMIC_REGNUM)
-	fprintf (outfile, " %d virtual-stack-dynamic", regno);
+	fprintf (outfile, " virtual-stack-dynamic");
       else if (regno == VIRTUAL_OUTGOING_ARGS_REGNUM)
-	fprintf (outfile, " %d virtual-outgoing-args", regno);
+	fprintf (outfile, " virtual-outgoing-args");
       else if (regno == VIRTUAL_CFA_REGNUM)
-	fprintf (outfile, " %d virtual-cfa", regno);
+	fprintf (outfile, " virtual-cfa");
       else if (regno == VIRTUAL_PREFERRED_STACK_BOUNDARY_REGNUM)
-	fprintf (outfile, " %d virtual-preferred-stack-boundary",
-		 regno);
+	fprintf (outfile, " virtual-preferred-stack-boundary");
       else
-	fprintf (outfile, " %d virtual-reg-%d", regno,
-		 regno-FIRST_VIRTUAL_REGISTER);
+	fprintf (outfile, " virtual-reg-%d", regno-FIRST_VIRTUAL_REGISTER);
     }
   else
 #endif
@@ -410,6 +427,10 @@ print_rtx_operand_code_r (const_rtx in_rtx)
 static void
 print_rtx_operand_code_u (const_rtx in_rtx, int idx)
 {
+  /* Don't print insn UIDs in compact mode, apart from in LABEL_REFs.  */
+  if (flag_compact && GET_CODE (in_rtx) != LABEL_REF)
+    return;
+
   if (XEXP (in_rtx, idx) != NULL)
     {
       rtx sub = XEXP (in_rtx, idx);
@@ -492,7 +513,7 @@ print_rtx_operand (const_rtx in_rtx, int idx)
       if (! flag_simple)
 	fprintf (outfile, " ");
       fprintf (outfile, HOST_WIDE_INT_PRINT_DEC, XWINT (in_rtx, idx));
-      if (! flag_simple)
+      if (! flag_simple && !flag_compact)
 	fprintf (outfile, " [" HOST_WIDE_INT_PRINT_HEX "]",
 		 (unsigned HOST_WIDE_INT) XWINT (in_rtx, idx));
       break;
@@ -533,6 +554,9 @@ print_rtx_operand (const_rtx in_rtx, int idx)
       break;
 
     case 'B':
+      /* Don't print basic block ids in compact mode.  */
+      if (flag_compact)
+	break;
 #ifndef GENERATOR_FILE
       if (XBBDEF (in_rtx, idx))
 	fprintf (outfile, " %i", XBBDEF (in_rtx, idx)->index);
@@ -575,7 +599,12 @@ print_rtx (const_rtx in_rtx)
     }
 
   /* Print name of expression code.  */
-  if (flag_simple && CONST_INT_P (in_rtx))
+
+  /* In compact mode, prefix the code of insns with "c",
+     giving "cinsn", "cnote" etc.  */
+  if (flag_compact && is_a <const rtx_insn *, const struct rtx_def> (in_rtx))
+    fprintf (outfile, "(c%s", GET_RTX_NAME (GET_CODE (in_rtx)));
+  else if (flag_simple && CONST_INT_P (in_rtx))
     fputc ('(', outfile);
   else
     fprintf (outfile, "(%s", GET_RTX_NAME (GET_CODE (in_rtx)));
@@ -639,13 +668,16 @@ print_rtx (const_rtx in_rtx)
     idx = 5;
 #endif
 
-  if (INSN_CHAIN_CODE_P (GET_CODE (in_rtx)))
-    {
-      if (flag_dump_unnumbered)
-	fprintf (outfile, " #");
-      else
-	fprintf (outfile, " %d", INSN_UID (in_rtx));
-    }
+  /* For insns, print the INSN_UID.
+     In compact mode, we only print the INSN_UID of CODE_LABELs.  */
+  if (!flag_compact || GET_CODE (in_rtx) == CODE_LABEL)
+    if (INSN_CHAIN_CODE_P (GET_CODE (in_rtx)))
+      {
+	if (flag_dump_unnumbered)
+	  fprintf (outfile, " #");
+	else
+	  fprintf (outfile, " %d", INSN_UID (in_rtx));
+      }
 
   /* Get the format string and skip the first elements if we have handled
      them already.  */
@@ -704,7 +736,8 @@ print_rtx (const_rtx in_rtx)
 #endif
 
     case CODE_LABEL:
-      fprintf (outfile, " [%d uses]", LABEL_NUSES (in_rtx));
+      if (!flag_compact)
+	fprintf (outfile, " [%d uses]", LABEL_NUSES (in_rtx));
       switch (LABEL_KIND (in_rtx))
 	{
 	  case LABEL_NORMAL: break;
diff --git a/gcc/print-rtl.h b/gcc/print-rtl.h
index 9cad683..7a1dcaf 100644
--- a/gcc/print-rtl.h
+++ b/gcc/print-rtl.h
@@ -36,6 +36,6 @@ extern void print_insn (pretty_printer *pp, const rtx_insn *x, int verbose);
 extern void rtl_dump_bb_for_graph (pretty_printer *, basic_block);
 extern const char *str_pattern_slim (const_rtx);
 
-extern void print_rtx_function (FILE *file, function *fn);
+extern void print_rtx_function (FILE *file, function *fn, bool compact);
 
 #endif  // GCC_PRINT_RTL_H
-- 
1.8.5.3