[gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Martin Jambor]

Hi,

On Mon, Nov 11, 2013 at 10:15:24AM -0700, Aldy Hernandez wrote:
> On 11/07/13 10:58, Martin Jambor wrote:
> 
> >Sorry for the delay.  I'd just like to re-iterate one comment from my
> >previous email which is that I do not think tree-sra.c should export
> >any function to the outside world apart from the entry points of the
> >passes (yes, there is already build_ref_for_offset which I admit is
> >entirely my creation but that should be moved elswhere too).
> >
> >So please put sra_ipa_get_adjustment_candidate to ipa-prop.c.  I see
> >that it requires get_ssa_base_param to be moved there as well but
> >since IPA-SRA uses it in different places, it would need exporting
> >too, which would be weird because it does not really do anything with
> >parameters.  Since the function is so trivial, I would even suggest
> >introducing another private copy to ipa-prop.c (and leaving the
> >original without the new parameter).  Alternatively, you can move the
> >function to tree.c but for that it looks too specialized.
> 
> Done.
> 
> Note that I didn't have to move
> ipa_sra_modify_function_body, since it wasn't used outside of
> tree-sra.c, and omp-low.c has its own version.  Instead I just
> removed it from ipa-prop.h where I had inadvertently placed it.
> 

Thanks.  I only have two comments about ipa_get_adjustment_candidate:

> commit c7602b7b88a9e85c7e3076e38d471be5bc728089
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Mon Nov 11 10:10:47 2013 -0700
> 
>     	* ipa-prop.c (get_ssa_base_param): New.
>     	* ipa-prop.h (ipa_modify_expr): Rename from ipa_sra_modify_expr.
>     	Remove ipa_sra_modify_function_body.
>     	(ipa_get_adjustment_candidate): Rename from
>     	sra_ipa_get_adjustment_candidate.
>     	* omp-low.c (ipa_simd_modify_stmt_ops): Rename
>     	sra_ipa_get_adjustment_candidate to ipa_get_adjustment_candidate.
>     	* tree-sra.c (get_ssa_base_param): Remove default_def argument.
>     	(create_access): Remove lass argument to get_ssa_base_param.
>     	(disqualify_base_of_expr): Same.
>     	(sra_ipa_get_adjustment_candidate): Rename to
>     	ipa_get_adjustment_candidate and move to ipa-prop.c.
>     	(sra_ipa_modify_expr): Rename to ipa_modify_expr and move to
>     	ipa-prop.c.
>     	(sra_ipa_modify_assign): Rename sra_ipa_modify_expr to
>     	ipa_modify_expr.
>     	(ipa_sra_modify_function_body): Same.
> 
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 76e9b61..042d623 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -3756,6 +3756,124 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, gimple stmt,
>    free_dominance_info (CDI_DOMINATORS);
>  }
>  
> +/* If the expression *EXPR should be replaced by a reduction of a parameter, do
> +   so.  ADJUSTMENTS is a pointer to a vector of adjustments.  CONVERT
> +   specifies whether the function should care about type incompatibility the
> +   current and new expressions.  If it is false, the function will leave
> +   incompatibility issues to the caller.  Return true iff the expression
> +   was modified. */
> +
> +bool
> +ipa_modify_expr (tree *expr, bool convert,
> +		 ipa_parm_adjustment_vec adjustments)
> +{
> +  struct ipa_parm_adjustment *cand
> +    = ipa_get_adjustment_candidate (expr, &convert, adjustments, false);
> +  if (!cand)
> +    return false;
> +
> +  tree src;
> +  if (cand->by_ref)
> +    src = build_simple_mem_ref (cand->new_decl);
> +  else
> +    src = cand->new_decl;
> +
> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +    {
> +      fprintf (dump_file, "About to replace expr ");
> +      print_generic_expr (dump_file, *expr, 0);
> +      fprintf (dump_file, " with ");
> +      print_generic_expr (dump_file, src, 0);
> +      fprintf (dump_file, "\n");
> +    }
> +
> +  if (convert && !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
> +    {
> +      tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
> +      *expr = vce;
> +    }
> +  else
> +    *expr = src;
> +  return true;
> +}
> +
> +/* If T is an SSA_NAME, return NULL if it is not a default def or
> +   return its base variable if it is.  If IGNORE_DEFAULT_DEF is true,
> +   the base variable is always returned, regardless if it is a default
> +   def.  Return T if it is not an SSA_NAME.  */
> +
> +static tree
> +get_ssa_base_param (tree t, bool ignore_default_def)
> +{
> +  if (TREE_CODE (t) == SSA_NAME)
> +    {
> +      if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
> +	return SSA_NAME_VAR (t);
> +      else
> +	return NULL_TREE;
> +    }
> +  return t;
> +}
> +
> +/* Given an expression, return an adjustment entry specifying the
> +   transformation to be done on EXPR.  If no suitable adjustment entry
> +   was found, returns NULL.
> +
> +   If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
> +   default def, otherwise bail on them.
> +
> +   If CONVERT is non-NULL,

This is not true, convert can be set even when the function might
return NULL afterwards.

> + this function will set *CONVERT if the
> +   expression provided is a component reference that must be converted
> +   upon return.  ADJUSTMENTS is the adjustments vector.  */
> +
> +ipa_parm_adjustment *
> +ipa_get_adjustment_candidate (tree *&expr, bool *convert,
> +			      ipa_parm_adjustment_vec adjustments,
> +			      bool ignore_default_def)

I find changing parameters passed by reference confusing and
error-prone, I would very much prefer if this was "tree **expr".
Either way, you should document that expr can be changed in the
function comment.

The patch is OK with me but please at least fix the comments.

Thanks,

Martin


> +{
> +  if (TREE_CODE (*expr) == BIT_FIELD_REF
> +      || TREE_CODE (*expr) == IMAGPART_EXPR
> +      || TREE_CODE (*expr) == REALPART_EXPR)
> +    {
> +      expr = &TREE_OPERAND (*expr, 0);
> +      if (convert)
> +	*convert = true;
> +    }
> +
> +  HOST_WIDE_INT offset, size, max_size;
> +  tree base = get_ref_base_and_extent (*expr, &offset, &size, &max_size);
> +  if (!base || size == -1 || max_size == -1)
> +    return NULL;
> +
> +  if (TREE_CODE (base) == MEM_REF)
> +    {
> +      offset += mem_ref_offset (base).low * BITS_PER_UNIT;
> +      base = TREE_OPERAND (base, 0);
> +    }
> +
> +  base = get_ssa_base_param (base, ignore_default_def);
> +  if (!base || TREE_CODE (base) != PARM_DECL)
> +    return NULL;
> +
> +  struct ipa_parm_adjustment *cand = NULL;
> +  unsigned int len = adjustments.length ();
> +  for (unsigned i = 0; i < len; i++)
> +    {
> +      struct ipa_parm_adjustment *adj = &adjustments[i];
> +
> +      if (adj->base == base
> +	  && (adj->offset == offset || adj->op == IPA_PARM_OP_REMOVE))
> +	{
> +	  cand = adj;
> +	  break;
> +	}
> +    }
> +
> +  if (!cand || cand->op == IPA_PARM_OP_COPY || cand->op == IPA_PARM_OP_REMOVE)
> +    return NULL;
> +  return cand;
> +}
> +
>  /* Return true iff BASE_INDEX is in ADJUSTMENTS more than once.  */
>  
>  static bool