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

[James Greenhalgh]

Sorry for the top-post, mobile client as I am out of office.

OK,

Thanks,
James

________________________________________
From: Jakub Jelinek <jakub@redhat.com>
Sent: 09 February 2018 14:39:27
To: Richard Earnshaw; James Greenhalgh; Marcus Shawcroft
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] cortex-a57-fma-steering.c fixes (PR target/84272)

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.

--- 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]);
+}

        Jakub