[RFC] Handle unary pass-through jump functions for ipa-vrp

Martin Jambor mjambor@suse.cz
Thu Oct 27 12:11:00 GMT 2016


Hi,

On Tue, Oct 25, 2016 at 10:18:25AM +1100, kugan wrote:
> Hi,
> 
> Attached RFC patch handles unary pass-through jump functions for ipa-vrp
> such that in the following case:
> 
> int bar (int j)
> {
>   foo (~j);
>   foo (abs (j));
>   foo (j);
>   return 0;
> }

Thanks for working on this.  Although I cannot approve your patches, I
do have a few comments inline:


> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 1dc5cb6..d0dc3d7 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -122,6 +122,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "ipa-inline.h"
>  #include "ipa-utils.h"
>  #include "tree-ssa-ccp.h"
> +#include "gimple.h"
>  
>  template <typename valtype> class ipcp_value;
>  
> @@ -1221,7 +1222,12 @@ ipa_get_jf_pass_through_result (struct ipa_jump_func *jfunc, tree input)
>  
>    if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
>      return input;
> -  if (!is_gimple_ip_invariant (input))
> +
> +  if (!is_gimple_ip_invariant (input)
> +      /* TODO: Unary expressions are not handles in ipa constant
> +	 propagation. */
> +     || (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> +	  == tcc_unary))
>      return NULL_TREE;
>  
>    if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> @@ -1845,7 +1851,8 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
>  static bool
>  propagate_vr_accross_jump_function (cgraph_edge *cs,
>  				    ipa_jump_func *jfunc,
> -				    struct ipcp_param_lattices *dest_plats)
> +				    struct ipcp_param_lattices *dest_plats,
> +				    tree param_type)

Please also add a brief description of the new parameter to the
function comment.

>  {
>    struct ipcp_param_lattices *src_lats;
>    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
> @@ -1863,18 +1870,43 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>  
>        if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
>  	return dest_lat->meet_with (src_lats->m_value_range);
> +      else if (param_type
> +	       && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> +		   == tcc_unary))
> +	{
> +	  value_range vr;
> +	  memset (&vr, 0, sizeof (vr));
> +	  tree operand_type = ipa_get_type (caller_info, src_idx);
> +
> +	  if (src_lats->m_value_range.bottom_p ())
> +	    return false;
> +
> +	  extract_range_from_unary_expr (&vr,
> +					 ipa_get_jf_pass_through_operation (jfunc),

Here you get over 80 characters of text width.  Tough to deal with, I
know, but please fix :-)

> +					 param_type,
> +					 &src_lats->m_value_range.m_vr,
> +					 operand_type);
> +	  return dest_lat->meet_with (&vr);
> +	}
>      }
> -  else if (jfunc->type == IPA_JF_CONST)
> +  else if (param_type
> +	   && jfunc->type == IPA_JF_CONST)
>      {
>        tree val = ipa_get_jf_constant (jfunc);
>        if (TREE_CODE (val) == INTEGER_CST)
>  	{
> +	  value_range vr;
>  	  if (TREE_OVERFLOW_P (val))
>  	    val = drop_tree_overflow (val);
> -	  jfunc->vr_known = true;
> -	  jfunc->m_vr.type = VR_RANGE;
> -	  jfunc->m_vr.min = val;
> -	  jfunc->m_vr.max = val;
> +
> +	  vr.type = VR_RANGE;
> +	  vr.min = val;
> +	  vr.max = val;
> +	  vr.equiv = NULL;
> +	  extract_range_from_unary_expr (&jfunc->m_vr,
> +					 NOP_EXPR,
> +					 param_type,
> +					 &vr, TREE_TYPE (val));

Do I understand it correctly that extract_range_from_unary_expr deals
with any potential type conversions better (compared to what you did
before here)?

Side note: I wonder whether it is a good idea to store the
intermediary result to the jump function.  I'd prefer if we did that
only after actually making transformation decisions, if at all.  But
that is only a minor nit and not something that needs to be addressed
as a part of this change.

>  	  return dest_lat->meet_with (&jfunc->m_vr);
>  	}
>      }
> @@ -2220,6 +2252,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>      {
>        struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>        struct ipcp_param_lattices *dest_plats;
> +      tree param_type = ipa_get_callee_param_type (cs, i);
>  
>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>        if (availability == AVAIL_INTERPOSABLE)
> @@ -2236,7 +2269,8 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>  						       dest_plats);
>  	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
>  	    ret |= propagate_vr_accross_jump_function (cs,
> -						       jump_func, dest_plats);
> +						       jump_func, dest_plats,
> +						       param_type);
>  	  else
>  	    ret |= dest_plats->m_value_range.set_to_bottom ();
>  	}
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 1629870..9147b27 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -446,6 +446,18 @@ ipa_set_jf_simple_pass_through (struct ipa_jump_func *jfunc, int formal_id,
>    jfunc->value.pass_through.agg_preserved = agg_preserved;
>  }
>  
> +/* Set JFUNC to be an unary pass through jump function.  */
> +
> +static void
> +ipa_set_jf_unary_pass_through (struct ipa_jump_func *jfunc, int formal_id,
> +			       enum tree_code operation)
> +{
> +  jfunc->type = IPA_JF_PASS_THROUGH;
> +  jfunc->value.pass_through.operand = NULL_TREE;
> +  jfunc->value.pass_through.formal_id = formal_id;
> +  jfunc->value.pass_through.operation = operation;
> +  jfunc->value.pass_through.agg_preserved = false;
> +}
>  /* Set JFUNC to be an arithmetic pass through jump function.  */
>  
>  static void
> @@ -850,18 +862,17 @@ parm_preserved_before_stmt_p (struct ipa_func_body_info *fbi, int index,
>  }
>  
>  /* If STMT is an assignment that loads a value from an parameter declaration,
> -   return the index of the parameter in ipa_node_params which has not been
> -   modified.  Otherwise return -1.  */
> +   return the index of the parameter in ipa_node_params.  Otherwise return -1.  */
>  
>  static int
> -load_from_unmodified_param (struct ipa_func_body_info *fbi,
> -			    vec<ipa_param_descriptor> descriptors,
> -			    gimple *stmt)
> +load_from_param (struct ipa_func_body_info *fbi,
> +		 vec<ipa_param_descriptor> descriptors,
> +		 gimple *stmt)

This is the only thing I dislike in this patch, I would like to keep
the ability to really know that we are looking at a load of an
unmodified parameter for cases when it is required/useful (e.g. in
ipa_load_from_parm_agg which I suppose "works" because there are not
that many unary operations on pointers).

Please either provide two distinct functions (sharing the common parts
in a load_from_param_1 function) or add a parameter that will
differentiate between unmodified and unary-op-modified parameters.
One way could be that the that parameter would be pointer to an enum
tree_code in which the function will return the OP for you if non-NULL
and will insist on a GIMPLE_SINGLE_RHS if it is NULL.

>  {
>    int index;
>    tree op1;
>  
> -  if (!gimple_assign_single_p (stmt))
> +  if (!is_gimple_assign (stmt))
>      return -1;

Even in the modifying case, I still think that you want to check any
operation embedded in the statement is GIMPLE_UNARY_RHS or
GIMPLE_SINGLE_RHS.

>  
>    op1 = gimple_assign_rhs1 (stmt);
> @@ -1025,7 +1036,7 @@ ipa_load_from_parm_agg (struct ipa_func_body_info *fbi,
>        */
>  
>        gimple *def = SSA_NAME_DEF_STMT (TREE_OPERAND (base, 0));
> -      index = load_from_unmodified_param (fbi, descriptors, def);
> +      index = load_from_param (fbi, descriptors, def);
>      }
>  
>    if (index >= 0)
> @@ -1109,6 +1120,7 @@ compute_complex_assign_jump_func (struct
> ipa_func_body_info *fbi,

After you add this new capability to the function, its comment will
need an update.

>    tree op1, tc_ssa, base, ssa;
>    bool reverse;
>    int index;
> +  gimple *stmt2 = stmt;
>  
>    op1 = gimple_assign_rhs1 (stmt);
>  
> @@ -1117,13 +1129,16 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
>        if (SSA_NAME_IS_DEFAULT_DEF (op1))
>  	index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
>        else
> -	index = load_from_unmodified_param (fbi, info->descriptors,
> -					    SSA_NAME_DEF_STMT (op1));
> +	{
> +	  index = load_from_param (fbi, info->descriptors,
> +				   SSA_NAME_DEF_STMT (op1));
> +	  stmt2 = SSA_NAME_DEF_STMT (op1);
> +	}
>        tc_ssa = op1;
>      }
>    else
>      {
> -      index = load_from_unmodified_param (fbi, info->descriptors, stmt);
> +      index = load_from_param (fbi, info->descriptors, stmt);
>        tc_ssa = gimple_assign_lhs (stmt);
>      }
>  
> @@ -1147,6 +1162,13 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
>  	  bool agg_p = parm_ref_data_pass_through_p (fbi, index, call, tc_ssa);
>  	  ipa_set_jf_simple_pass_through (jfunc, index, agg_p);
>  	}
> +      else if (is_gimple_assign (stmt2)
> +	       && (gimple_expr_code (stmt2) != NOP_EXPR)
> +	       && (TREE_CODE_CLASS (gimple_expr_code (stmt2)) == tcc_unary))
> +	{
> +	  ipa_set_jf_unary_pass_through (jfunc, index,
> +					 gimple_assign_rhs_code (stmt2));
> +	}
>        return;
>      }
>  
> @@ -1595,7 +1617,7 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
>      }
>  }
>  
> -static tree
> +tree
>  ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>  {
>    int n;
> @@ -4663,6 +4685,10 @@ ipa_write_jump_function (struct output_block *ob,
>  	  bp_pack_value (&bp, jump_func->value.pass_through.agg_preserved, 1);
>  	  streamer_write_bitpack (&bp);
>  	}
> +      else if (TREE_CODE_CLASS (jump_func->value.pass_through.operation) == tcc_unary)

This is quite clearly over 80 characters wide.


Apart from the above, I'll be happy to see this in.

Thanks,

Martin



More information about the Gcc-patches mailing list