diff mbox

Fix PR bootstrap/58509

Message ID 4797238.rpsq4UIoxF@polaris
State New
Headers show

Commit Message

Eric Botcazou Sept. 27, 2013, 11:17 a.m. UTC 
Hi,

this fixes the ICE during the build of the Ada runtime on the SPARC, a fallout 
of the recent inliner changes:
  http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01033.html

The ICE is triggered because the ldd peephole merges an MEM with MEM_NOTRAP_P 
and a contiguous MEM without MEM_NOTRAP_P, keeping the MEM_NOTRAP_P flag on 
the result.  As a consequence, an EH edge is eliminated and a BB is orphaned.

I think this shows that my above inliner patch was too gross: when you have 
successive inlining, you can quickly end up with a mess of trapping and non-
trapping memory accesses for the same object.  So the attached seriously 
refines it, restricting it to parameters with reference type and leaning 
towards being less conservative.  Again, this should only affect Ada.

Tested on x86_64-suse-linux, OK for the mainline?


2013-09-27  Eric Botcazou  <ebotcazou@adacore.com>

	PR bootstrap/58509
	* ipa-prop.h (get_ancestor_addr_info): Declare.
	* ipa-prop.c (get_ancestor_addr_info): Make public.
	* tree-inline.c (is_parm): Rename into...
	(is_ref_parm): ...this.
	(is_based_on_ref_parm): New predicate.
	(remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if
	a parameter with reference type has been remapped and the result is
	not based on another parameter with reference type.
	(copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF.


2013-09-27  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/specs/opt1.ads: New test.

Comments

Richard Biener Oct. 8, 2013, 10:11 a.m. UTC | #1 
On Fri, Sep 27, 2013 at 1:17 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this fixes the ICE during the build of the Ada runtime on the SPARC, a fallout
> of the recent inliner changes:
>   http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01033.html
>
> The ICE is triggered because the ldd peephole merges an MEM with MEM_NOTRAP_P
> and a contiguous MEM without MEM_NOTRAP_P, keeping the MEM_NOTRAP_P flag on
> the result.  As a consequence, an EH edge is eliminated and a BB is orphaned.
>
> I think this shows that my above inliner patch was too gross: when you have
> successive inlining, you can quickly end up with a mess of trapping and non-
> trapping memory accesses for the same object.  So the attached seriously
> refines it, restricting it to parameters with reference type and leaning
> towards being less conservative.  Again, this should only affect Ada.
>
> Tested on x86_64-suse-linux, OK for the mainline?

This is getting somewhat gross ... what about clearing all TREE_NO_TRAPs
on inlining?

Otherwise I think the "proper" way is to teach passes that moving loads/stores
eventually has to clear TREE_NO_TRAP ... (a reason that for example
VRP cannot set TREE_NO_TRAP on dereferences of pointers that have
a non-NULL range...).

Richard.


> 2013-09-27  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR bootstrap/58509
>         * ipa-prop.h (get_ancestor_addr_info): Declare.
>         * ipa-prop.c (get_ancestor_addr_info): Make public.
>         * tree-inline.c (is_parm): Rename into...
>         (is_ref_parm): ...this.
>         (is_based_on_ref_parm): New predicate.
>         (remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if
>         a parameter with reference type has been remapped and the result is
>         not based on another parameter with reference type.
>         (copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF.
>
>
> 2013-09-27  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/specs/opt1.ads: New test.
>
>
> --
> Eric Botcazou
diff mbox

Patch

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 202912)
+++ tree-inline.c	(working copy)
@@ -751,10 +751,11 @@  copy_gimple_bind (gimple stmt, copy_body
   return new_bind;
 }
 
-/* Return true if DECL is a parameter or a SSA_NAME for a parameter.  */
+/* Return true if DECL is a parameter with reference type or a SSA_NAME
+  for a parameter with reference type.  */
 
 static bool
-is_parm (tree decl)
+is_ref_parm (tree decl)
 {
   if (TREE_CODE (decl) == SSA_NAME)
     {
@@ -763,7 +764,40 @@  is_parm (tree decl)
 	return false;
     }
 
-  return (TREE_CODE (decl) == PARM_DECL);
+  return (TREE_CODE (decl) == PARM_DECL
+	  && TREE_CODE (TREE_TYPE (decl)) == REFERENCE_TYPE);
+}
+
+/* Return true if DECL is based on a parameter with reference type or a
+   SSA_NAME for a parameter with with reference type.  */
+
+static bool
+is_based_on_ref_parm (tree decl)
+{
+  HOST_WIDE_INT offset;
+  tree obj, expr;
+  gimple def_stmt;
+
+  /* First the easy case.  */
+  if (is_ref_parm (decl))
+    return true;
+
+  /* Then look for an SSA name whose defining statement is of the form:
+
+      D.1718_7 = &parm_2(D)->f1;
+
+     where parm_2 is a parameter with reference type.  */
+  if (TREE_CODE (decl) != SSA_NAME)
+    return false;
+  def_stmt = SSA_NAME_DEF_STMT (decl);
+  if (!def_stmt)
+    return false;
+
+  expr = get_ancestor_addr_info (def_stmt, &obj, &offset);
+  if (!expr)
+    return false;
+
+  return is_ref_parm (TREE_OPERAND (expr, 0));
 }
 
 /* Remap the GIMPLE operand pointed to by *TP.  DATA is really a
@@ -865,12 +899,13 @@  remap_gimple_op_r (tree *tp, int *walk_s
 	  TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
 	  TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
 	  TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old);
-	  /* We cannot propagate the TREE_THIS_NOTRAP flag if we have
-	     remapped a parameter as the property might be valid only
-	     for the parameter itself.  */
+	  /* We cannot always propagate the TREE_THIS_NOTRAP flag if we have
+	     remapped a parameter with reference type as the property may be
+	     valid only for the parameter.  */
 	  if (TREE_THIS_NOTRAP (old)
-	      && (!is_parm (TREE_OPERAND (old, 0))
-		  || (!id->transform_parameter && is_parm (ptr))))
+	      && (!is_ref_parm (TREE_OPERAND (old, 0))
+		  || !id->transform_parameter
+		  || is_based_on_ref_parm (ptr)))
 	    TREE_THIS_NOTRAP (*tp) = 1;
 	  *walk_subtrees = 0;
 	  return NULL;
@@ -1092,12 +1127,13 @@  copy_tree_body_r (tree *tp, int *walk_su
 		      TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
 		      TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
 		      TREE_READONLY (*tp) = TREE_READONLY (old);
-		      /* We cannot propagate the TREE_THIS_NOTRAP flag if we
-			 have remapped a parameter as the property might be
-			 valid only for the parameter itself.  */
+		      /* We cannot always propagate the TREE_THIS_NOTRAP flag
+			 if we have remapped a parameter with reference type as
+			 the property may be valid only for the parameter.  */
 		      if (TREE_THIS_NOTRAP (old)
-			  && (!is_parm (TREE_OPERAND (old, 0))
-			      || (!id->transform_parameter && is_parm (ptr))))
+			  && (!is_ref_parm (TREE_OPERAND (old, 0))
+			      || !id->transform_parameter
+			      || is_based_on_ref_parm (ptr)))
 		        TREE_THIS_NOTRAP (*tp) = 1;
 		    }
 		}
@@ -1118,12 +1154,13 @@  copy_tree_body_r (tree *tp, int *walk_su
 	  TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
 	  TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
 	  TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old);
-	  /* We cannot propagate the TREE_THIS_NOTRAP flag if we have
-	     remapped a parameter as the property might be valid only
-	     for the parameter itself.  */
+	  /* We cannot always propagate the TREE_THIS_NOTRAP flag if we have
+	     remapped a parameter with reference type as the property may be
+	     valid only for the parameter.  */
 	  if (TREE_THIS_NOTRAP (old)
-	      && (!is_parm (TREE_OPERAND (old, 0))
-		  || (!id->transform_parameter && is_parm (ptr))))
+	      && (!is_ref_parm (TREE_OPERAND (old, 0))
+		  || !id->transform_parameter
+		  || is_based_on_ref_parm (ptr)))
 	    TREE_THIS_NOTRAP (*tp) = 1;
 	  *walk_subtrees = 0;
 	  return NULL;
Index: ipa-prop.h
===================================================================
--- ipa-prop.h	(revision 202912)
+++ ipa-prop.h	(working copy)
@@ -693,6 +693,7 @@  tree ipa_value_from_jfunc (struct ipa_no
 unsigned int ipcp_transform_function (struct cgraph_node *node);
 void ipa_dump_param (FILE *, struct ipa_node_params *info, int i);
 
+tree get_ancestor_addr_info (gimple, tree *, HOST_WIDE_INT *);
 
 /* From tree-sra.c:  */
 tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, tree,
Index: ipa-prop.c
===================================================================
--- ipa-prop.c	(revision 202912)
+++ ipa-prop.c	(working copy)
@@ -1078,7 +1078,7 @@  compute_complex_assign_jump_func (struct
    handled components and the MEM_REF itself is stored into *OFFSET.  The whole
    RHS stripped off the ADDR_EXPR is stored into *OBJ_P.  */
 
-static tree
+tree
 get_ancestor_addr_info (gimple assign, tree *obj_p, HOST_WIDE_INT *offset)
 {
   HOST_WIDE_INT size, max_size;