rtx_writer: avoid printing trailing default values

On Fri, 2016-11-04 at 18:51 +0100, Bernd Schmidt wrote:
> Here's something simpler. Only very lightly tested, but isn't
> something
> like this all that's needed? Could decide whether to apply it to
> expr_list etc. as well.
> 
> Index: print-rtl.c
> ===================================================================
> --- print-rtl.c	(revision 241233)
> +++ print-rtl.c	(working copy)
> @@ -697,7 +697,12 @@ print_rtx (const_rtx in_rtx)
> 
>     /* Get the format string and skip the first elements if we have
> handled
>        them already.  */
> -  for (; idx < GET_RTX_LENGTH (GET_CODE (in_rtx)); idx++)
> +  int limit = GET_RTX_LENGTH (GET_CODE (in_rtx));
> +  if (flag_compact
(as of r241586 this is now "m_compact")

> +      && INSN_CHAIN_CODE_P (GET_CODE (in_rtx))
> +      && XEXP (in_rtx, limit - 1) == NULL_RTX)
> +    limit--;
> +  for (; idx < limit; idx++)
>       print_rtx_operand (in_rtx, idx);
> 
>     switch (GET_CODE (in_rtx))

Thanks.

This seems to assume that the final code in the fmt string can be
accessed via XEXP (in_rtx, limit - 1), which if RTL checking is
enabled requires that the code be either 'e' or 'u'.

This isn't the case for JUMP_INSN, JUMP_TABLE_DATA, BARRIER
(all code '0'), CODE_LABEL (code 's') and NOTE (code 'i').

Also, we might want to omit the REG_NOTES from, say a JUMP_INSN, which
is the *penultimate* operand - for example, it doesn't omit the
trailing (nil) from the cjump_insn in test_uncond_jump.

That said, this is much simpler than my patch, so I used it as the
basis for the following: it uses the same approach as your patch, but
loops backwards from the end of the format string (rather than just once)
until it finds a non-default value, using a new function
"operand_has_default_value_p" to handle the logic for that.

With this, compact dumps omit the trailing (nil) for both regular insns
and for JUMP_INSNs, and omits the surplus newline seen in my earlier patch.

It also appears removes the trailing (nil) from expr_list.

Bootstrap&regrtest in progress.

OK for trunk if it passes?

gcc/ChangeLog:
	* print-rtl.c (operand_has_default_value_p): New function.
	(rtx_writer::print_rtx): In compact mode, omit trailing operands
	that have the default values.
	* rtl-tests.c (selftest::test_dumping_insns): Remove empty
	label string from expected dump.
	(seltest::test_uncond_jump): Remove trailing "(nil)" for REG_NOTES
	from expected dump.
---
 gcc/print-rtl.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 gcc/rtl-tests.c |  5 ++---
 2 files changed, 46 insertions(+), 4 deletions(-)

On 11/04/2016 08:14 PM, David Malcolm wrote:
>
> With this, compact dumps omit the trailing (nil) for both regular insns
> and for JUMP_INSNs, and omits the surplus newline seen in my earlier patch.
>
> It also appears removes the trailing (nil) from expr_list.
>
> Bootstrap&regrtest in progress.
>
> OK for trunk if it passes?

This seems OK, with something left to be addressed perhaps in a followup:

> +	case JUMP_INSN:
> +	  return JUMP_LABEL (in_rtx) == NULL_RTX;

Weren't we going to omit JUMP_LABELs in compact mode? If so, we'll need 
a m_compact test here eventually as well.


Bernd

On Fri, 2016-11-04 at 19:53 +0100, Bernd Schmidt wrote:
> On 11/04/2016 08:14 PM, David Malcolm wrote:
> > 
> > With this, compact dumps omit the trailing (nil) for both regular
> > insns
> > and for JUMP_INSNs, and omits the surplus newline seen in my
> > earlier patch.
> > 
> > It also appears removes the trailing (nil) from expr_list.
> > 
> > Bootstrap&regrtest in progress.
> > 
> > OK for trunk if it passes?
> 
> This seems OK, with something left to be addressed perhaps in a
> followup:
> 
> > +	case JUMP_INSN:
> > +	  return JUMP_LABEL (in_rtx) == NULL_RTX;
> 
> Weren't we going to omit JUMP_LABELs in compact mode? If so, we'll
> need 
> a m_compact test here eventually as well.

We already omit JUMP_LABELs in compact mode, in
rtx_writer::print_rtx_operand_code_0, but we need this test to pass so
that we can potentially omit earlier (nil) operands in a JUMP_INSN e.g.
REG_NOTES.

Given that, perhaps this test should read:

	case JUMP_INSN:	

	  /* JUMP_LABELs are always omitted in compact mode, so treat
	     any value here as omittable, so that earlier operands can
	     potentially be omitted also.  */
	  return true;

or:

          return m_compact;

if we want to support calling operand_has_default_value_p in non
-compact mode.

OK with one of those changes?  (assuming successful testing)

On 11/04/2016 08:25 PM, David Malcolm wrote:

>           return m_compact;

Ok with this one plus a comment.


Bernd