cortex-a57-fma-steering.c fixes (PR target/84272)

Message ID 20180209143927.GH5867@tucnak
State New
Headers show
Series
  • cortex-a57-fma-steering.c fixes (PR target/84272)
Related show

Commit Message

Jakub Jelinek Feb. 9, 2018, 2:39 p.m.
Hi!

As mentioned in the PR, in the cortex-a57-fma-steering.c optimization pass
we may use fma_node/fma_root_node data after they were deleted and leak
leaf nodes because of an early continue; (the loop with
to_process.safe_push (*child_iter); in the body will do nothing if list
is empty, but freeing it is important).

The following patch fixes it, Thomas as the pass author looked at the patch
and provided feedback in the PR and Kyrill tested it on aarch64-linux.

Ok for trunk?

2018-02-09  Jakub Jelinek  <jakub@redhat.com>

	PR target/84272
	* config/aarch64/cortex-a57-fma-steering.c (fma_forest::merge_forest):
	Use ++iter rather than iter++ for std::list iterators.
	(func_fma_steering::dfs): Likewise.  Don't delete nodes right away,
	defer deleting them until all nodes in the forest are processed.  Do
	free even leaf nodes.  Change to_process into auto_vec.

	* g++.dg/opt/pr84272.C: New test.


	Jakub

Comments

James Greenhalgh Feb. 16, 2018, 9:20 a.m. | #1
Sorry for the top-post, mobile client as I am out of office.

OK,

Thanks,
James

Patch

--- gcc/config/aarch64/cortex-a57-fma-steering.c.jj	2018-02-09 06:44:24.990811997 +0100
+++ gcc/config/aarch64/cortex-a57-fma-steering.c	2018-02-09 13:03:51.741447714 +0100
@@ -406,7 +406,7 @@  fma_forest::merge_forest (fma_forest *ot
 
   /* Update root nodes' pointer to forest.  */
   for (other_root_iter = other_roots->begin ();
-       other_root_iter != other_roots->end (); other_root_iter++)
+       other_root_iter != other_roots->end (); ++other_root_iter)
     (*other_root_iter)->set_forest (this);
 
   /* Remove other_forest from the list of forests and move its tree roots in
@@ -847,14 +847,13 @@  func_fma_steering::dfs (void (*process_f
 			void (*process_node) (fma_forest *, fma_node *),
 			bool free)
 {
-  vec<fma_node *> to_process;
+  auto_vec<fma_node *> to_process;
+  auto_vec<fma_node *> to_free;
   std::list<fma_forest *>::iterator forest_iter;
 
-  to_process.create (0);
-
   /* For each forest.  */
   for (forest_iter = this->m_fma_forests.begin ();
-       forest_iter != this->m_fma_forests.end (); forest_iter++)
+       forest_iter != this->m_fma_forests.end (); ++forest_iter)
     {
       std::list<fma_root_node *>::iterator root_iter;
 
@@ -863,7 +862,7 @@  func_fma_steering::dfs (void (*process_f
 
       /* For each tree root in this forest.  */
       for (root_iter = (*forest_iter)->get_roots ()->begin ();
-	   root_iter != (*forest_iter)->get_roots ()->end (); root_iter++)
+	   root_iter != (*forest_iter)->get_roots ()->end (); ++root_iter)
 	{
 	  if (process_root)
 	    process_root (*forest_iter, *root_iter);
@@ -881,28 +880,30 @@  func_fma_steering::dfs (void (*process_f
 	  if (process_node)
 	    process_node (*forest_iter, node);
 
-	  /* Absence of children might indicate an alternate root of a *chain*.
-	     It's ok to skip it here as the chain will be renamed when
-	     processing the canonical root for that chain.  */
-	  if (node->get_children ()->empty ())
-	    continue;
-
 	  for (child_iter = node->get_children ()->begin ();
-	       child_iter != node->get_children ()->end (); child_iter++)
+	       child_iter != node->get_children ()->end (); ++child_iter)
 	    to_process.safe_push (*child_iter);
+
+	  /* Defer freeing so that the process_node callback can access the
+	     parent and children of the node being processed.  */
 	  if (free)
+	    to_free.safe_push (node);
+	}
+
+      if (free)
+	{
+	  delete *forest_iter;
+
+	  while (!to_free.is_empty ())
 	    {
+	      fma_node *node = to_free.pop ();
 	      if (node->root_p ())
 		delete static_cast<fma_root_node *> (node);
 	      else
 		delete node;
 	    }
 	}
-      if (free)
-	delete *forest_iter;
     }
-
-  to_process.release ();
 }
 
 /* Build the dependency trees of FMUL and FMADD/FMSUB instructions.  */
--- gcc/testsuite/g++.dg/opt/pr84272.C.jj	2018-02-09 12:59:28.215636324 +0100
+++ gcc/testsuite/g++.dg/opt/pr84272.C	2018-02-09 12:59:28.215636324 +0100
@@ -0,0 +1,23 @@ 
+// PR target/84272
+// { dg-do compile }
+// { dg-options "-O2" }
+// { dg-additional-options "-march=armv8-a -mtune=cortex-a57" { target aarch64-*-* } }
+
+struct A
+{
+  float b, c;
+  A ();
+  A (float, float, float);
+  float operator * (A)
+  {
+    float d = b * b + c * c;
+    return d;
+  }
+};
+
+void
+foo ()
+{
+  A g[1];
+  A h (0, 0, h * g[2]);
+}