Re: [patch, avr] Take 2: Fix PR64331: insn output and insn length computation rely on REG_DEAD notes.

[Georg-Johann Lay <avr at gjlay dot de>]

Am 02/23/2015 um 11:53 AM schrieb Georg-Johann Lay:
> 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.

Let me drop that...  Problem was that df relies on cfg which is down after a 
specific point (after pass .*free_cfg).

Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to 
rectify notes.  The pass is scheduled right before cfg does down (right before 
.*free_cfg) so that cfg and hence df machinery is available.

Regression tests look fine and for the test case the patches produce correct 
code and correct insn length.

Ok for trunk and 4.9?

Johann


gcc/
	PR target/64331
	* config/avr/avr.c (context.h, tree-pass.h): Include them.
	(avr_pass_data_recompute_notes): New static variable.
	(avr_pass_recompute_notes): New class.
	(avr_register_passes): New static function.
	(avr_option_override): Call it.

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

Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 220964)
+++ config/avr/avr.c	(working copy)
@@ -81,6 +81,8 @@
 #include "basic-block.h"
 #include "df.h"
 #include "builtins.h"
+#include "context.h"
+#include "tree-pass.h"
 
 /* Maximal allowed offset for an address in the LD command */
 #define MAX_LD_OFFSET(MODE) (64 - (signed)GET_MODE_SIZE (MODE))
@@ -329,6 +331,55 @@ avr_to_int_mode (rtx x)
 }
 
 
+static const pass_data avr_pass_data_recompute_notes =
+{
+  RTL_PASS,      // type
+  "",            // name (will be patched)
+  OPTGROUP_NONE, // optinfo_flags
+  TV_DF_SCAN,    // tv_id
+  0,             // properties_required
+  0,             // properties_provided
+  0,             // properties_destroyed
+  0,             // todo_flags_start
+  TODO_df_finish | TODO_df_verify // todo_flags_finish
+};
+
+
+class avr_pass_recompute_notes : public rtl_opt_pass
+{
+public:
+  avr_pass_recompute_notes (gcc::context *ctxt, const char *name)
+    : rtl_opt_pass (avr_pass_data_recompute_notes, ctxt)
+  {
+    this->name = name;
+  }
+
+  virtual unsigned int execute (function*)
+  {
+    df_note_add_problem ();
+    df_analyze ();
+
+    return 0;
+  }
+}; // avr_pass_recompute_notes
+
+
+static void
+avr_register_passes (void)
+{
+  /* This avr-specific pass (re)computes insn notes, in particular REG_DEAD
+     notes which are used by `avr.c::reg_unused_after' and branch offset
+     computations.  These notes must be correct, i.e. there must be no
+     dangling REG_DEAD notes; otherwise wrong code might result, cf. PR64331.
+
+     DF needs (correct) CFG, hence right before free_cfg is the last
+     opportunity to rectify notes.  */
+
+  register_pass (new avr_pass_recompute_notes (g, "avr-notes-free-cfg"),
+                 PASS_POS_INSERT_BEFORE, "*free_cfg", 1);
+}
+
+
 /* Implement `TARGET_OPTION_OVERRIDE'.  */
 
 static void
@@ -411,6 +462,11 @@ if (!avr_current_device->macro
   init_machine_status = avr_init_machine_status;
 
   avr_log_set_avr_log();
+
+  /* Register some avr-specific pass(es).  There is no canonical place for
+     pass registration.  This function is convenient.  */
+
+  avr_register_passes ();
 }
 
 /* Function to set up the backend function structure.  */
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,37 @@
+/* { 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;
+}

Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 220963)
+++ config/avr/avr.c	(working copy)
@@ -51,6 +51,8 @@
 #include "target-def.h"
 #include "params.h"
 #include "df.h"
+#include "context.h"
+#include "tree-pass.h"
 
 /* Maximal allowed offset for an address in the LD command */
 #define MAX_LD_OFFSET(MODE) (64 - (signed)GET_MODE_SIZE (MODE))
@@ -285,6 +287,58 @@ avr_to_int_mode (rtx x)
 }
 
 
+static const pass_data avr_pass_data_recompute_notes =
+{
+  RTL_PASS,       // type
+  "",             // name (will be patched)
+  OPTGROUP_NONE,  // optinfo_flags
+  false,          // has_gate
+  true,           // has_execute
+  TV_DF_SCAN,     // tv_id
+  0,              // properties_required
+  0,              // properties_provided
+  0,              // properties_destroyed
+  0,              // todo_flags_start
+  // todo_flags_finish
+  TODO_df_finish | TODO_verify_rtl_sharing | TODO_verify_flow
+};
+
+
+class avr_pass_recompute_notes : public rtl_opt_pass
+{
+public:
+  avr_pass_recompute_notes (gcc::context *ctxt, const char *name)
+    : rtl_opt_pass (avr_pass_data_recompute_notes, ctxt)
+  {
+    this->name = name;
+  }
+
+  unsigned int execute (void)
+  {
+    df_note_add_problem ();
+    df_analyze ();
+
+    return 0;
+  }
+}; // avr_pass_recompute_notes
+
+
+static void
+avr_register_passes (void)
+{
+  /* This avr-specific pass (re)computes insn notes, in particular REG_DEAD
+     notes which are used by `avr.c::reg_unused_after' and branch offset
+     computations.  These notes must be correct, i.e. there must be no
+     dangling REG_DEAD notes; otherwise wrong code might result, cf. PR64331.
+
+     DF needs (correct) CFG, hence right before free_cfg is the last
+     opportunity to rectify notes.  */
+
+  register_pass (new avr_pass_recompute_notes (g, "avr-notes-free-cfg"),
+                 PASS_POS_INSERT_BEFORE, "*free_cfg", 1);
+}
+
+
 /* Implement `TARGET_OPTION_OVERRIDE'.  */
 
 static void
@@ -346,6 +400,11 @@ avr_option_override (void)
   init_machine_status = avr_init_machine_status;
 
   avr_log_set_avr_log();
+
+  /* Register some avr-specific pass(es).  There is no canonical place for
+     pass registration.  This function is convenient.  */
+
+  avr_register_passes ();
 }
 
 /* Function to set up the backend function structure.  */
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,37 @@
+/* { 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;
+}