[avr] Fix PR64331: insn output and insn length computation rely on REG_DEAD notes.

This patch fixes PR64331 which produced wrong code because of outdated (too 
many) REG_DEAD notes.  These notes are not (re)computed per default, hence do 
the computation by hand each time avr.c:reg_unused_after is called in a 
different pass.

Ok to apply?

Johann


gcc/
	PR target/64331
	* config/avr/avr.c (reg_unused_after): Recompute insn notes if the
	pass changed since the last (re)computation.
	* config/avr/avr.h (machine_function.dead_notes_pass_number): New
	component.

gcc/testsuite/
	PR target/64331
	* gcc.target/avr/torture/pr64331.c: New run test.

On Mon, Feb 23, 2015 at 11:53:52AM +0100, Georg-Johann Lay wrote:
> This patch fixes PR64331 which produced wrong code because of outdated (too
> many) REG_DEAD notes.  These notes are not (re)computed per default, hence
> do the computation by hand each time avr.c:reg_unused_after is called in a
> different pass.
> 
> Ok to apply?
> 

I was testing a similar patch a while back. Mine had a call to
df_finish_pass after calling df_analyze, and I didn't "cache" the pass
number. I remember running into ICEs
building libgcc after that - from df_verify, IIRC.

Do you know why df_finish_pass isn't needed in this case? AFAICT, all it
appears to do is to clean up some stuff and enable a verification flag.

Regards
Senthil

> Johann
> 
> 
> gcc/
> 	PR target/64331
> 	* config/avr/avr.c (reg_unused_after): Recompute insn notes if the
> 	pass changed since the last (re)computation.
> 	* config/avr/avr.h (machine_function.dead_notes_pass_number): New
> 	component.
> 
> gcc/testsuite/
> 	PR target/64331
> 	* gcc.target/avr/torture/pr64331.c: New run test.
> 

> Index: config/avr/avr.c
> ===================================================================
> --- config/avr/avr.c	(revision 220738)
> +++ config/avr/avr.c	(working copy)
> @@ -51,6 +51,7 @@
>  #include "target-def.h"
>  #include "params.h"
>  #include "df.h"
> +#include "tree-pass.h"	/* for current_pass */
>  
>  /* Maximal allowed offset for an address in the LD command */
>  #define MAX_LD_OFFSET(MODE) (64 - (signed)GET_MODE_SIZE (MODE))
> @@ -7862,8 +7863,22 @@ avr_adjust_insn_length (rtx insn, int le
>  int
>  reg_unused_after (rtx insn, rtx reg)
>  {
> +  if (cfun->machine->dead_notes_pass_number
> +      != current_pass->static_pass_number)
> +    {
> +      /* `dead_or_set_p' needs correct REG_DEAD notes, cf. PR64331.  Hence
> +         recompute them each time we find ourselves in a different pass.
> +         `reg_unused_after' is used during final for insn output, and in
> +         some earlier passes it is involved in insn length computations.  */
> +
> +      df_note_add_problem ();
> +      df_analyze ();
> +      
> +      cfun->machine->dead_notes_pass_number = current_pass->static_pass_number;
> +    }
> +
>    return (dead_or_set_p (insn, reg)
> -	  || (REG_P(reg) && _reg_unused_after (insn, reg)));
> +	  || (REG_P (reg) && _reg_unused_after (insn, reg)));
>  }
>  
>  /* Return nonzero if REG is not used after INSN.
> Index: config/avr/avr.h
> ===================================================================
> --- config/avr/avr.h	(revision 220738)
> +++ config/avr/avr.h	(working copy)
> @@ -592,6 +592,10 @@ struct GTY(()) machine_function
>    /* 'true' if the above is_foo predicates are sanity-checked to avoid
>       multiple diagnose for the same function.  */
>    int attributes_checked_p;
> +
> +  /* The latest static pass number for which REG_DEAD notes have been
> +	 computed, cf. PR64331.  */
> +  int dead_notes_pass_number;
>  };
>  
>  /* AVR does not round pushes, but the existence of this macro is
> Index: testsuite/gcc.target/avr/torture/pr64331.c
> ===================================================================
> --- testsuite/gcc.target/avr/torture/pr64331.c	(revision 0)
> +++ testsuite/gcc.target/avr/torture/pr64331.c	(revision 0)
> @@ -0,0 +1,38 @@
> +/* { dg-do run } */
> +
> +typedef struct
> +{
> +  unsigned a, b;
> +} T2;
> +
> +
> +__attribute__((__noinline__, __noclone__))
> +void foo2 (T2 *t, int x)
> +{
> +  if (x != t->a)
> +    {
> +      t->a = x;
> +  
> +      if (x && x == t->b)
> +	t->a = 20;
> +    }
> +}
> +
> +
> +T2 t;
> +
> +int main (void)
> +{
> +  t.a = 1;
> +  t.b = 1234;
> +
> +  foo2 (&t, 1234);
> +
> +  if (t.a != 20)
> +    __builtin_abort();
> +
> +  __builtin_exit (0);
> +
> +  return 0;
> +}
> +

2015-02-23 13:53 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
> This patch fixes PR64331 which produced wrong code because of outdated (too
> many) REG_DEAD notes.  These notes are not (re)computed per default, hence
> do the computation by hand each time avr.c:reg_unused_after is called in a
> different pass.
>
> Ok to apply?
>
> Johann
>
>
> gcc/
>         PR target/64331
>         * config/avr/avr.c (reg_unused_after): Recompute insn notes if the
>         pass changed since the last (re)computation.
>         * config/avr/avr.h (machine_function.dead_notes_pass_number): New
>         component.
>
> gcc/testsuite/
>         PR target/64331
>         * gcc.target/avr/torture/pr64331.c: New run test.
>

Approved. Please apply.

Denis.