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®rtested 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
