This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH]: Merge reduced memory usage for memory tags from improved-aliasing-branch


Daniel Berlin <dberlin@dberlin.org> writes:


> -      if (!(TREE_STATIC (var) || DECL_EXTERNAL (var))
> -	  && (var_ann (var)->mem_tag_kind == NOT_A_TAG
> -	      || var_ann (var)->mem_tag_kind == STRUCT_FIELD)
> +      if (!(is_global_var (var))
> +	  && (!MTAG_P (var) || TREE_CODE (var) == STRUCT_FIELD_TAG)

The extra parentheses around the call to is_global_var are slighly
confusing.  Please remove them.


> -#define SSA_VAR_P(DECL) \
> -	(TREE_CODE (DECL) == VAR_DECL	\
> -	 || TREE_CODE (DECL) == PARM_DECL \
> -	 || TREE_CODE (DECL) == RESULT_DECL \
> -	 || (TREE_CODE (DECL) == SSA_NAME \
> -	     && (TREE_CODE (SSA_NAME_VAR (DECL)) == VAR_DECL \
> -		 || TREE_CODE (SSA_NAME_VAR (DECL)) == PARM_DECL \
> -		 || TREE_CODE (SSA_NAME_VAR (DECL)) == RESULT_DECL)))
> +#define SSA_VAR_P(DECL)							\
> +	(TREE_CODE (DECL) == VAR_DECL					\
> +	 || TREE_CODE (DECL) == PARM_DECL				\
> +	 || TREE_CODE (DECL) == RESULT_DECL				\
> +	 || MTAG_P (DECL)						\
> +	 || (TREE_CODE (DECL) == SSA_NAME				\
> +	     && (TREE_CODE (SSA_NAME_VAR (DECL)) == VAR_DECL		\
> +		 || TREE_CODE (SSA_NAME_VAR (DECL)) == PARM_DECL	\
> +		 || TREE_CODE (SSA_NAME_VAR (DECL)) == RESULT_DECL	\
> +		 || MTAG_P (SSA_NAME_VAR (DECL)))))

It's a little hard to be sure, but I think there is an extra tab on
the #define line.  Please correct if so.


> @@ -271,6 +271,9 @@ is_gimple_reg (tree t)
>    if (TREE_CODE (t) == SSA_NAME)
>      t = SSA_NAME_VAR (t);
>  
> +  if (MTAG_P (t))
> +    return false;
> +
>    if (!is_gimple_variable (t))
>      return false;
>  
> @@ -305,11 +308,12 @@ is_gimple_reg (tree t)
>    if (TREE_CODE (TREE_TYPE (t)) == COMPLEX_TYPE)
>      return DECL_COMPLEX_GIMPLE_REG_P (t);
>  
> +
>    /* Some compiler temporaries are created to be used exclusively in
>       virtual operands (currently memory tags and sub-variables).
>       These variables should never be considered GIMPLE registers.  */
>    if (DECL_ARTIFICIAL (t) && (ann = var_ann (t)) != NULL)
> -    return ann->mem_tag_kind == NOT_A_TAG;
> +    return !MTAG_P (t);

Extraneous blank line above the comment.  Please remove it.

And this change in general doesn't make sense.  You already tested
MTAG_P (t) at the start of the function, so you know it is not false.
My guess is that you can remove this test entirely.  Please check
this.


> @@ -1500,7 +1499,7 @@ may_alias_p (tree ptr, HOST_WIDE_INT mem
>  
>    m_ann = var_ann (mem);
>  
> -  gcc_assert (m_ann->mem_tag_kind == TYPE_TAG);
> +  gcc_assert (TREE_CODE (mem) == TYPE_MEMORY_TAG);

Can't you just remove the m_ann variable entirely?  If so, please do.


> @@ -1769,16 +1793,9 @@ get_nmt_for (tree ptr)
>  {
>    struct ptr_info_def *pi = get_ptr_info (ptr);
>    tree tag = pi->name_mem_tag;
> -
> +  

Adds undesired trailing whitespace.  Please don't.


>    if (tag == NULL_TREE)
>      tag = create_memory_tag (TREE_TYPE (TREE_TYPE (ptr)), false);
> -
> -  /* If PTR is a PARM_DECL, it points to a global variable or malloc,
> -     then its name tag should be considered a global variable.  */
> -  if (TREE_CODE (SSA_NAME_VAR (ptr)) == PARM_DECL
> -      || pi->pt_global_mem)
> -    mark_call_clobbered (tag);
> -
>    return tag;
>  }

This patch isn't in the ChangeLog, and it doesn't seem to be
mechanical.  Are you sure you meant to include it in this patch?
Please send it separately.


> Index: tree.def
> ===================================================================
> --- tree.def	(revision 107947)
> +++ tree.def	(working copy)
> @@ -357,6 +357,9 @@ DEFTREECODE (CONST_DECL, "const_decl", t
>  DEFTREECODE (PARM_DECL, "parm_decl", tcc_declaration, 0)
>  DEFTREECODE (TYPE_DECL, "type_decl", tcc_declaration, 0)
>  DEFTREECODE (RESULT_DECL, "result_decl", tcc_declaration, 0)
> +DEFTREECODE (STRUCT_FIELD_TAG, "struct_field_tag", tcc_declaration, 0)
> +DEFTREECODE (NAME_MEMORY_TAG, "name_memory_tag", tcc_declaration, 0)
> +DEFTREECODE (TYPE_MEMORY_TAG, "type_memory_tag", tcc_declaration, 0)

Please add a comment here explaining what these are.


> Index: tree-dfa.c
> ===================================================================
> --- tree-dfa.c	(revision 107947)
> +++ tree-dfa.c	(working copy)
> @@ -638,11 +638,15 @@ add_referenced_var (tree var, struct wal
>  	*slot = (void *) var;
>        
>        referenced_var_insert (DECL_UID (var), var);
> -
> +      

Adds undesired trailing whitespace.  Please don't.


> @@ -454,9 +462,12 @@ print_node (FILE *file, const char *pref
>  
>        print_node_brief (file, "context", DECL_CONTEXT (node), indent + 4);
>  
> -      print_node_brief (file, "attributes",
> -			DECL_ATTRIBUTES (node), indent + 4);
> -      
> +      if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))	
> +	{
> +	  print_node_brief (file, "attributes",
> +			    DECL_ATTRIBUTES (node), indent + 4);
> +	  print_node_brief (file, "initial", DECL_INITIAL (node), indent + 4);
> +	}
>        if (CODE_CONTAINS_STRUCT (code, TS_DECL_WRTL))
>  	{
>  	  print_node_brief (file, "abstract_origin",
> @@ -467,7 +478,6 @@ print_node (FILE *file, const char *pref
>  	  print_node (file, "arguments", DECL_ARGUMENT_FLD (node), indent + 4);
>  	  print_node (file, "result", DECL_RESULT_FLD (node), indent + 4);
>  	}
> -      print_node_brief (file, "initial", DECL_INITIAL (node), indent + 4);
>  
>        lang_hooks.print_decl (file, node, indent);

Is anybody going to complain about this move of the "initial" field to
earlier in the printout?  (I don't care myself.)


> Index: tree-ssa-structalias.c
> ===================================================================
> --- tree-ssa-structalias.c	(revision 107947)
> +++ tree-ssa-structalias.c	(working copy)
> @@ -2036,7 +2036,7 @@ bitpos_of_field (const tree fdecl)
>        || TREE_CODE (DECL_FIELD_BIT_OFFSET (fdecl)) != INTEGER_CST)
>      return -1;
>    
> -  return (tree_low_cst (DECL_FIELD_OFFSET (fdecl), 1) * 8) 
> +  return (tree_low_cst (DECL_FIELD_OFFSET (fdecl), 1) * BITS_PER_UNIT) 
>           + tree_low_cst (DECL_FIELD_BIT_OFFSET (fdecl), 1);
>  }

This patch is neither mechanical nor in the ChangeLog.  Please send it
separately.


The patch is approved with those changes.  Technically I can't approve
the patch to cp/ptree.c, but it is only one line, and I think counts
as obvious even without the rest of the patch.

Thanks.

:REVIEWMAIL:

Ian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]