diff mbox series

[PR,89330] Avoid adding dead speculative edges to inlinig heap

Message ID ri6ef89t99q.fsf@suse.cz
State New
Headers show
Series [PR,89330] Avoid adding dead speculative edges to inlinig heap | expand

Commit Message

Martin Jambor Feb. 15, 2019, 2:29 p.m. UTC
Hi,

Martin discovered that inliner was adding deleted call graph edges to
its heap when supposedly processing newly discovered direct edges.  The
problem is that a new edge created in the speculation part of the
indirect inlining machinery created speculative edges that were
immediately afterwards removed by check_speculations() after it figured
out the edge is not speculation_useful_p().

The fix below avoids creating such non-speculation_useful_p edges in the
first place.  The edge is not useful because it cannot be inlined
because the callee calls comdat local functions.  I had to split
can_inline_edge_p into two functions to allow perform the caller and
callee checks before actually creating an edge.

I think this is safe and beneficial to commit now, maybe with the
exception of the newly added assert in add_new_edges_to_heap, since
inlining apparently can cope with such nonsensical edges in the heap.
But in that case I'd add the assert in the next stage1.

Bootstrapped and tested on x86_64-linux.  IIUC, Martin even
LTO-bootstrapped it.  OK for trunk?

Thanks,

Martin



2019-02-15  Martin Jambor  <mjambor@suse.cz>

	PR ipa/89330
	* ipa-inline.c (can_inline_edge_p): Move most of the checks...
	(call_not_inlinable_p): ...this new function.
	(add_new_edges_to_heap): Assert a caller is known.
	* ipa-inline.h (call_not_inlinable_p): Declare.
	* ipa-prop.c: Include ipa-inline.h
	(try_make_edge_direct_virtual_call): Create speculative edges only
	if there is any chance of inlining them.

	testsuite/
	* g++.dg/lto/pr89330_[01].C: New test.
---
 gcc/ipa-inline.c                     | 128 ++++++++++++---------------
 gcc/ipa-inline.h                     |   4 +-
 gcc/ipa-prop.c                       |   8 +-
 gcc/testsuite/g++.dg/lto/pr89330_0.C |  50 +++++++++++
 gcc/testsuite/g++.dg/lto/pr89330_1.C |  36 ++++++++
 5 files changed, 154 insertions(+), 72 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/lto/pr89330_0.C
 create mode 100644 gcc/testsuite/g++.dg/lto/pr89330_1.C

Comments

Martin Liška June 6, 2019, 8:46 a.m. UTC | #1
Hi.

This is rebased version of the patch that Martin J. wrote.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Thanks,
Martin
Jan Hubicka June 6, 2019, 1:03 p.m. UTC | #2
My understanding is that the problematic situation is when we create 
speculation,
insert it to the priority queue and then decide it is useless.
This is done by speculation_useful_p and that checks more than just the 
fact whether
function is inlinable (it accepts targets declared as PURE/CONST but 
also punts on cold
calls).
It also checks inline limits. So won't it break when these things are 
not in sync?

Honza

Dne 2019-06-06 10:46, Martin Liška napsal:
> Hi.
> 
> This is rebased version of the patch that Martin J. wrote.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Thanks,
> Martin
diff mbox series

Patch

diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 360c3de3289..ae330943571 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -299,12 +299,60 @@  sanitize_attrs_match_for_inline_p (const_tree caller, const_tree callee)
       (opts_for_fn (caller->decl)->x_##flag		\
        != opts_for_fn (callee->decl)->x_##flag)
 
+/* Return CIF_OK if a call from CALLER to CALLEE is or would be inlineable.
+   Otherwise, return the reason why it cannot.  EARLY should be set when
+   deciding about early inlining.  */
+
+enum cgraph_inline_failed_t
+call_not_inlinable_p (cgraph_node *caller, cgraph_node *callee,
+		      bool early)
+{
+  enum availability avail;
+  caller = caller->global.inlined_to ? caller->global.inlined_to : caller;
+  callee = callee->ultimate_alias_target (&avail, caller);
+
+  if (!callee->definition)
+    return CIF_BODY_NOT_AVAILABLE;
+  if (!early && (!opt_for_fn (callee->decl, optimize)
+		 || !opt_for_fn (caller->decl, optimize)))
+    return CIF_FUNCTION_NOT_OPTIMIZED;
+  else if (callee->calls_comdat_local)
+    return CIF_USES_COMDAT_LOCAL;
+  else if (avail <= AVAIL_INTERPOSABLE)
+    return CIF_OVERWRITABLE;
+  /* Don't inline if the functions have different EH personalities.  */
+  else if (DECL_FUNCTION_PERSONALITY (caller->decl)
+	   && DECL_FUNCTION_PERSONALITY (callee->decl)
+	   && (DECL_FUNCTION_PERSONALITY (caller->decl)
+	       != DECL_FUNCTION_PERSONALITY (callee->decl)))
+    return CIF_EH_PERSONALITY;
+  /* TM pure functions should not be inlined into non-TM_pure
+     functions.  */
+  else if (is_tm_pure (callee->decl) && !is_tm_pure (caller->decl))
+    return CIF_UNSPECIFIED;
+  /* Check compatibility of target optimization options.  */
+  else if (!targetm.target_option.can_inline_p (caller->decl,
+						callee->decl))
+    return CIF_TARGET_OPTION_MISMATCH;
+  else if (ipa_fn_summaries->get (callee) == NULL
+	   || !ipa_fn_summaries->get (callee)->inlinable)
+    return CIF_FUNCTION_NOT_INLINABLE;
+  /* Don't inline a function with mismatched sanitization attributes. */
+  else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl))
+    return CIF_ATTRIBUTE_MISMATCH;
+  else if (callee->externally_visible
+	   && flag_live_patching == LIVE_PATCHING_INLINE_ONLY_STATIC)
+    return CIF_EXTERN_LIVE_ONLY_STATIC;
+  return CIF_OK;
+}
+
 /* Decide if we can inline the edge and possibly update
    inline_failed reason.  
    We check whether inlining is possible at all and whether
    caller growth limits allow doing so.  
 
-   if REPORT is true, output reason to the dump file. */
+   If REPORT is true, output reason to the dump file.  EARLY should be set when
+   deciding about early inlining.  */
 
 static bool
 can_inline_edge_p (struct cgraph_edge *e, bool report,
@@ -319,81 +367,22 @@  can_inline_edge_p (struct cgraph_edge *e, bool report,
       return false;
     }
 
-  bool inlinable = true;
-  enum availability avail;
-  cgraph_node *caller = e->caller->global.inlined_to
-		        ? e->caller->global.inlined_to : e->caller;
-  cgraph_node *callee = e->callee->ultimate_alias_target (&avail, caller);
+  enum cgraph_inline_failed_t fail_reason
+    = call_not_inlinable_p (e->caller, e->callee, early);
 
-  if (!callee->definition)
+
+  if (fail_reason != CIF_OK)
     {
-      e->inline_failed = CIF_BODY_NOT_AVAILABLE;
-      inlinable = false;
-    }
-  if (!early && (!opt_for_fn (callee->decl, optimize)
-		 || !opt_for_fn (caller->decl, optimize)))
-    {
-      e->inline_failed = CIF_FUNCTION_NOT_OPTIMIZED;
-      inlinable = false;
-    }
-  else if (callee->calls_comdat_local)
-    {
-      e->inline_failed = CIF_USES_COMDAT_LOCAL;
-      inlinable = false;
-    }
-  else if (avail <= AVAIL_INTERPOSABLE)
-    {
-      e->inline_failed = CIF_OVERWRITABLE;
-      inlinable = false;
+      e->inline_failed = fail_reason;
+      if (report)
+	report_inline_failed_reason (e);
     }
   /* All edges with call_stmt_cannot_inline_p should have inline_failed
      initialized to one of FINAL_ERROR reasons.  */
   else if (e->call_stmt_cannot_inline_p)
     gcc_unreachable ();
-  /* Don't inline if the functions have different EH personalities.  */
-  else if (DECL_FUNCTION_PERSONALITY (caller->decl)
-	   && DECL_FUNCTION_PERSONALITY (callee->decl)
-	   && (DECL_FUNCTION_PERSONALITY (caller->decl)
-	       != DECL_FUNCTION_PERSONALITY (callee->decl)))
-    {
-      e->inline_failed = CIF_EH_PERSONALITY;
-      inlinable = false;
-    }
-  /* TM pure functions should not be inlined into non-TM_pure
-     functions.  */
-  else if (is_tm_pure (callee->decl) && !is_tm_pure (caller->decl))
-    {
-      e->inline_failed = CIF_UNSPECIFIED;
-      inlinable = false;
-    }
-  /* Check compatibility of target optimization options.  */
-  else if (!targetm.target_option.can_inline_p (caller->decl,
-						callee->decl))
-    {
-      e->inline_failed = CIF_TARGET_OPTION_MISMATCH;
-      inlinable = false;
-    }
-  else if (ipa_fn_summaries->get (callee) == NULL
-	   || !ipa_fn_summaries->get (callee)->inlinable)
-    {
-      e->inline_failed = CIF_FUNCTION_NOT_INLINABLE;
-      inlinable = false;
-    }
-  /* Don't inline a function with mismatched sanitization attributes. */
-  else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl))
-    {
-      e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
-      inlinable = false;
-    }
-  else if (callee->externally_visible
-	   && flag_live_patching == LIVE_PATCHING_INLINE_ONLY_STATIC)
-    {
-      e->inline_failed = CIF_EXTERN_LIVE_ONLY_STATIC;
-      inlinable = false;
-    }
-  if (!inlinable && report)
-    report_inline_failed_reason (e);
-  return inlinable;
+
+  return fail_reason == CIF_OK;
 }
 
 /* Decide if we can inline the edge and possibly update
@@ -1627,6 +1616,7 @@  add_new_edges_to_heap (edge_heap_t *heap, vec<cgraph_edge *> new_edges)
     {
       struct cgraph_edge *edge = new_edges.pop ();
 
+      gcc_assert (edge->callee);
       gcc_assert (!edge->aux);
       if (edge->inline_failed
 	  && can_inline_edge_p (edge, true)
diff --git a/gcc/ipa-inline.h b/gcc/ipa-inline.h
index f6eb677d618..911df58d646 100644
--- a/gcc/ipa-inline.h
+++ b/gcc/ipa-inline.h
@@ -52,7 +52,9 @@  void free_growth_caches (void);
 /* In ipa-inline.c  */
 unsigned int early_inliner (function *fun);
 bool inline_account_function_p (struct cgraph_node *node);
-
+enum cgraph_inline_failed_t call_not_inlinable_p (cgraph_node *caller,
+						  cgraph_node *callee,
+						  bool early);
 
 /* In ipa-inline-transform.c  */
 bool inline_call (struct cgraph_edge *, bool, vec<cgraph_edge *> *, int *, bool,
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index d86c2f3db55..36ba91c3800 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -53,6 +53,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "domwalk.h"
 #include "builtins.h"
 #include "tree-cfgcleanup.h"
+#include "ipa-inline.h"
 
 /* Function summary where the parameter infos are actually stored. */
 ipa_node_params_t *ipa_node_params_sum = NULL;
@@ -3346,13 +3347,16 @@  try_make_edge_direct_virtual_call (struct cgraph_edge *ie,
 
   if (target)
     {
-      if (!possible_polymorphic_call_target_p
-	  (ie, cgraph_node::get_create (target)))
+      cgraph_node *target_node = cgraph_node::get_create (target);
+      if (!possible_polymorphic_call_target_p (ie, target_node))
 	{
 	  if (speculative)
 	    return NULL;
 	  target = ipa_impossible_devirt_target (ie, target);
 	}
+      if (speculative
+	  && call_not_inlinable_p (ie->caller, target_node, false))
+	return NULL;
       return ipa_make_edge_direct_to_target (ie, target, speculative);
     }
   else
diff --git a/gcc/testsuite/g++.dg/lto/pr89330_0.C b/gcc/testsuite/g++.dg/lto/pr89330_0.C
new file mode 100644
index 00000000000..10082f83412
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr89330_0.C
@@ -0,0 +1,50 @@ 
+// { dg-lto-do link }
+// { dg-lto-options { { -O3 -g -flto -shared -Wno-odr } } }
+
+namespace Inkscape {
+class Anchored {};
+namespace XML {
+enum NodeType {};
+class Node :Anchored {
+public:
+  virtual NodeType type() ;
+  virtual char name() ;
+  virtual int code() ;
+  virtual unsigned position() ;
+  virtual unsigned childCount() ;
+  virtual char content() ;
+  virtual char *attribute() const ;
+  virtual int attributeList() ;
+  virtual bool matchAttributeName() ;
+  virtual void setPosition() ;
+  virtual void setContent() ;
+  virtual void setAttribute() ;
+  virtual int document() ;
+  virtual int document() const ;
+  virtual Node *root() ;
+  virtual Node *root() const ;
+  virtual Node *parent() ;
+  virtual Node *next() const ;
+  virtual Node *firstChild() const ;
+
+};
+} } struct rdf_license_t {
+  };
+;
+class RDFImpl {
+;
+  rdf_license_t *getLicense();
+};
+static bool rdf_match_license(Inkscape::XML::Node const *repr) {
+  for (Inkscape::XML::Node *current = repr->firstChild(); current;
+       current->next()->attribute());
+  return 0;
+}
+rdf_license_t *RDFImpl::getLicense() {
+  Inkscape::XML::Node *repr ;
+  for (rdf_license_t *license ; license;
+       license) {
+    rdf_match_license(repr);
+  }
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/lto/pr89330_1.C b/gcc/testsuite/g++.dg/lto/pr89330_1.C
new file mode 100644
index 00000000000..5b999eee8d7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr89330_1.C
@@ -0,0 +1,36 @@ 
+typedef char gchar;
+namespace Inkscape {
+class Anchored {
+int _anchor;
+};
+namespace XML {
+enum NodeType {};
+class Node :Anchored {
+virtual NodeType type() ;
+  virtual char const *name() const ;
+  virtual int code() ;
+  virtual unsigned position() ;
+  virtual unsigned childCount() ;
+  virtual char content() ;
+  virtual char attribute() ;
+  virtual int attributeList() ;
+  virtual bool matchAttributeName() ;
+  virtual void setPosition() ;
+  virtual void setContent() ;
+  virtual int document() ;
+  virtual int document() const ;
+  virtual Node *root() ;
+  virtual Node *root() const ;
+  virtual Node *parent() ;
+  virtual Node *parent() const ;
+  virtual Node *next() ;
+  virtual Node const *next() const ;
+
+};
+class SimpleNode : virtual Node {
+char const *name() const;
+  Node *next() const { return _next; }
+  SimpleNode *_next;
+};
+gchar const *SimpleNode::name() const { return 0; }
+} }