Patchwork Fix PR rtl-optimization/54870

login
register
mail settings
Submitter Eric Botcazou
Date Oct. 16, 2012, 9:39 a.m.
Message ID <1690142.kYm8ZhDSIE@polaris>
Download mbox | patch
Permalink /patch/191760/
State New
Headers show

Comments

Eric Botcazou - Oct. 16, 2012, 9:39 a.m.
> As I'm not sure how to best do that I suggest we do a more proper RTL
> DSE hack by adding a 'libcall-call-escape'-set which we can add to
> instead of calling mark_addressable this late.  We need to add all
> partitions of a decl here, of course, and we need to query it from
> can_escape.

That doesn't pertain only to libcalls though, mark_addressable can also be 
called for regular calls if arguments are callee-copied.  And mark_addressable 
has also been called from expand_asm_operands for ages.

> Well, it just means that the enhanced DSE is fragile :/

Maybe, but the current implementation is the outcome of a discussion between 
Easwaran and you IIRC.  In any case, before attempting to rewrite everything, 
here is another approach that patches things up by extending the usage of the 
TREE_ADDRESSABLE bit from the vars in a partition to the artificial pointer to 
that partition, i.e. the bit is set on the artificial pointer if it is set on 
at least one variable in the partition.

Tested on x86_64-suse-linux.


	PR rtl-optimization/54870
	* tree.h (TREE_ADDRESSABLE): Document special usage of SSA_NAME.
	* cfgexpand.c (update_alias_info_with_stack_vars ): Set it on the
	SSA_NAME pointer that points to a partition if there is at least
	one variable with it set in the partition.
	* dse.c (local_variable_can_escape): New predicate.
	(can_escape): Call it.
	* gimplify.c (mark_addressable): If this is a partitioned decl, mark
	the SSA_NAME pointer that points to a partition as well.
Richard Guenther - Oct. 16, 2012, 11:42 a.m.
On Tue, Oct 16, 2012 at 11:39 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> As I'm not sure how to best do that I suggest we do a more proper RTL
>> DSE hack by adding a 'libcall-call-escape'-set which we can add to
>> instead of calling mark_addressable this late.  We need to add all
>> partitions of a decl here, of course, and we need to query it from
>> can_escape.
>
> That doesn't pertain only to libcalls though, mark_addressable can also be
> called for regular calls if arguments are callee-copied.  And mark_addressable
> has also been called from expand_asm_operands for ages.

Hm, I see ...

>> Well, it just means that the enhanced DSE is fragile :/
>
> Maybe, but the current implementation is the outcome of a discussion between
> Easwaran and you IIRC.  In any case, before attempting to rewrite everything,
> here is another approach that patches things up by extending the usage of the
> TREE_ADDRESSABLE bit from the vars in a partition to the artificial pointer to
> that partition, i.e. the bit is set on the artificial pointer if it is set on
> at least one variable in the partition.

Ok, I like that a lot more - can_escape now properly checks all partition vars
and the use of TREE_ADDRESSABLE on the SSA-NAME is a nice idea.

Less ideal is the mark_addressable change but I can't see any better
way to deal with late addressable markings ...

> Tested on x86_64-suse-linux.

... thus, ok!

Thanks,
Richard.

>
>         PR rtl-optimization/54870
>         * tree.h (TREE_ADDRESSABLE): Document special usage of SSA_NAME.
>         * cfgexpand.c (update_alias_info_with_stack_vars ): Set it on the
>         SSA_NAME pointer that points to a partition if there is at least
>         one variable with it set in the partition.
>         * dse.c (local_variable_can_escape): New predicate.
>         (can_escape): Call it.
>         * gimplify.c (mark_addressable): If this is a partitioned decl, mark
>         the SSA_NAME pointer that points to a partition as well.
>
>
> --
> Eric Botcazou

Patch

Index: tree.h
===================================================================
--- tree.h	(revision 192447)
+++ tree.h	(working copy)
@@ -484,9 +484,10 @@  struct GTY(()) tree_base {
 
        TREE_ADDRESSABLE in
            VAR_DECL, PARM_DECL, RESULT_DECL, FUNCTION_DECL, LABEL_DECL
+           SSA_NAME
            all types
            CONSTRUCTOR, IDENTIFIER_NODE
-           STMT_EXPR, it means we want the result of the enclosed expression
+           STMT_EXPR
 
        CALL_EXPR_TAILCALL in
            CALL_EXPR
@@ -1085,15 +1086,18 @@  extern void omp_clause_range_check_faile
 /* In VAR_DECL, PARM_DECL and RESULT_DECL nodes, nonzero means address
    of this is needed.  So it cannot be in a register.
    In a FUNCTION_DECL it has no meaning.
-   In CONSTRUCTOR nodes, it means object constructed must be in memory.
    In LABEL_DECL nodes, it means a goto for this label has been seen
    from a place outside all binding contours that restore stack levels.
+   In an artificial SSA_NAME that points to a stack partition with at least
+   two variables, it means that at least one variable has TREE_ADDRESSABLE.
    In ..._TYPE nodes, it means that objects of this type must be fully
    addressable.  This means that pieces of this object cannot go into
    register parameters, for example.  If this a function type, this
    means that the value must be returned in memory.
+   In CONSTRUCTOR nodes, it means object constructed must be in memory.
    In IDENTIFIER_NODEs, this means that some extern decl for this name
-   had its address taken.  That matters for inline functions.  */
+   had its address taken.  That matters for inline functions.
+   In a STMT_EXPR, it means we want the result of the enclosed expression.  */
 #define TREE_ADDRESSABLE(NODE) ((NODE)->base.addressable_flag)
 
 /* Set on a CALL_EXPR if the call is in a tail position, ie. just before the
Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 192447)
+++ cfgexpand.c	(working copy)
@@ -635,6 +635,8 @@  update_alias_info_with_stack_vars (void)
 					   (void *)(size_t) uid)) = part;
 	  *((tree *) pointer_map_insert (cfun->gimple_df->decls_to_pointers,
 					 decl)) = name;
+	  if (TREE_ADDRESSABLE (decl))
+	    TREE_ADDRESSABLE (name) = 1;
 	}
 
       /* Make the SSA name point to all partition members.  */
Index: dse.c
===================================================================
--- dse.c	(revision 192447)
+++ dse.c	(working copy)
@@ -989,7 +989,32 @@  delete_dead_store_insn (insn_info_t insn
   insn_info->wild_read = false;
 }
 
-/* Check if EXPR can possibly escape the current function scope.  */
+/* Return whether DECL, a local variable, can possibly escape the current
+   function scope.  */
+
+static bool
+local_variable_can_escape (tree decl)
+{
+  if (TREE_ADDRESSABLE (decl))
+    return true;
+
+  /* If this is a partitioned variable, we need to consider all the variables
+     in the partition.  This is necessary because a store into one of them can
+     be replaced with a store into another and this may not change the outcome
+     of the escape analysis.  */
+  if (cfun->gimple_df->decls_to_pointers != NULL)
+    {
+      void *namep
+	= pointer_map_contains (cfun->gimple_df->decls_to_pointers, decl);
+      if (namep)
+	return TREE_ADDRESSABLE (*(tree *)namep);
+    }
+
+  return false;
+}
+
+/* Return whether EXPR can possibly escape the current function scope.  */
+
 static bool
 can_escape (tree expr)
 {
@@ -998,7 +1023,11 @@  can_escape (tree expr)
     return true;
   base = get_base_address (expr);
   if (DECL_P (base)
-      && !may_be_aliased (base))
+      && !may_be_aliased (base)
+      && !(TREE_CODE (base) == VAR_DECL
+	   && !DECL_EXTERNAL (base)
+	   && !TREE_STATIC (base)
+	   && local_variable_can_escape (base)))
     return false;
   return true;
 }
Index: gimplify.c
===================================================================
--- gimplify.c	(revision 192447)
+++ gimplify.c	(working copy)
@@ -116,6 +116,19 @@  mark_addressable (tree x)
       && TREE_CODE (x) != RESULT_DECL)
     return;
   TREE_ADDRESSABLE (x) = 1;
+
+  /* Also mark the artificial SSA_NAME that points to the partition of X.  */
+  if (TREE_CODE (x) == VAR_DECL
+      && !DECL_EXTERNAL (x)
+      && !TREE_STATIC (x)
+      && cfun->gimple_df != NULL
+      && cfun->gimple_df->decls_to_pointers != NULL)
+    {
+      void *namep
+	= pointer_map_contains (cfun->gimple_df->decls_to_pointers, x); 
+      if (namep)
+	TREE_ADDRESSABLE (*(tree *)namep) = 1;
+    }
 }
 
 /* Return a hash value for a formal temporary table entry.  */