Re: [PATCH] Only accept BUILT_IN_NORMAL stringops for interesting_stringop_to_profile_p

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Aug 20, 2015 at 11:31 AM, Yangfei (Felix) <felix.yang@huawei.com> wrote:
> Thanks for the comments.  Attached please find the updated patch.  OK?

Ok.

Thanks,
Richard.

>
> Index: gcc/value-prof.c
> ===================================================================
> --- gcc/value-prof.c    (revision 141081)
> +++ gcc/value-prof.c    (working copy)
> @@ -209,7 +209,6 @@ gimple_add_histogram_value (struct function *fun,
>    hist->fun = fun;
>  }
>
> -
>  /* Remove histogram HIST from STMT's histogram list.  */
>
>  void
> @@ -234,7 +233,6 @@ gimple_remove_histogram_value (struct function *fu
>    free (hist);
>  }
>
> -
>  /* Lookup histogram of type TYPE in the STMT.  */
>
>  histogram_value
> @@ -389,6 +387,7 @@ stream_out_histogram_value (struct output_block *o
>    if (hist->hvalue.next)
>      stream_out_histogram_value (ob, hist->hvalue.next);
>  }
> +
>  /* Dump information about HIST to DUMP_FILE.  */
>
>  void
> @@ -488,7 +487,6 @@ gimple_duplicate_stmt_histograms (struct function
>      }
>  }
>
> -
>  /* Move all histograms associated with OSTMT to STMT.  */
>
>  void
> @@ -529,7 +527,6 @@ visit_hist (void **slot, void *data)
>    return 1;
>  }
>
> -
>  /* Verify sanity of the histograms.  */
>
>  DEBUG_FUNCTION void
> @@ -594,7 +591,6 @@ free_histograms (void)
>      }
>  }
>
> -
>  /* The overall number of invocations of the counter should match
>     execution count of basic block.  Report it as error rather than
>     internal error as it might mean that user has misused the profile
> @@ -638,7 +634,6 @@ check_counter (gimple stmt, const char * name,
>    return false;
>  }
>
> -
>  /* GIMPLE based transformations. */
>
>  bool
> @@ -697,7 +692,6 @@ gimple_value_profile_transformations (void)
>    return changed;
>  }
>
> -
>  /* Generate code for transformation 1 (with parent gimple assignment
>     STMT and probability of taking the optimal path PROB, which is
>     equivalent to COUNT/ALL within roundoff error).  This generates the
> @@ -859,6 +853,7 @@ gimple_divmod_fixed_value_transform (gimple_stmt_i
>     probability of taking the optimal path PROB, which is equivalent to COUNT/ALL
>     within roundoff error).  This generates the result into a temp and returns
>     the temp; it does not replace or alter the original STMT.  */
> +
>  static tree
>  gimple_mod_pow2 (gimple stmt, int prob, gcov_type count, gcov_type all)
>  {
> @@ -938,6 +933,7 @@ gimple_mod_pow2 (gimple stmt, int prob, gcov_type
>  }
>
>  /* Do transform 2) on INSN if applicable.  */
> +
>  static bool
>  gimple_mod_pow2_value_transform (gimple_stmt_iterator *si)
>  {
> @@ -1540,15 +1536,15 @@ gimple_ic_transform (gimple_stmt_iterator *gsi)
>    return true;
>  }
>
> -/* Return true if the stringop CALL with FNDECL shall be profiled.
> -   SIZE_ARG be set to the argument index for the size of the string
> -   operation.
> -*/
> +/* Return true if the stringop CALL shall be profiled.  SIZE_ARG be
> +   set to the argument index for the size of the string operation.  */
> +
>  static bool
> -interesting_stringop_to_profile_p (tree fndecl, gimple call, int *size_arg)
> +interesting_stringop_to_profile_p (gimple call, int *size_arg)
>  {
> -  enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl);
> +  enum built_in_function fcode;
>
> +  fcode = DECL_FUNCTION_CODE (gimple_call_fndecl (call));
>    if (fcode != BUILT_IN_MEMCPY && fcode != BUILT_IN_MEMPCPY
>        && fcode != BUILT_IN_MEMSET && fcode != BUILT_IN_BZERO)
>      return false;
> @@ -1573,7 +1569,7 @@ static bool
>      }
>  }
>
> -/* Convert   stringop (..., vcall_size)
> +/* Convert stringop (..., vcall_size)
>     into
>     if (vcall_size == icall_size)
>       stringop (..., icall_size);
> @@ -1590,11 +1586,9 @@ gimple_stringop_fixed_value (gimple vcall_stmt, tr
>    basic_block cond_bb, icall_bb, vcall_bb, join_bb;
>    edge e_ci, e_cv, e_iv, e_ij, e_vj;
>    gimple_stmt_iterator gsi;
> -  tree fndecl;
>    int size_arg;
>
> -  fndecl = gimple_call_fndecl (vcall_stmt);
> -  if (!interesting_stringop_to_profile_p (fndecl, vcall_stmt, &size_arg))
> +  if (!interesting_stringop_to_profile_p (vcall_stmt, &size_arg))
>      gcc_unreachable ();
>
>    cond_bb = gimple_bb (vcall_stmt);
> @@ -1673,11 +1667,11 @@ gimple_stringop_fixed_value (gimple vcall_stmt, tr
>
>  /* Find values inside STMT for that we want to measure histograms for
>     division/modulo optimization.  */
> +
>  static bool
>  gimple_stringops_transform (gimple_stmt_iterator *gsi)
>  {
>    gimple stmt = gsi_stmt (*gsi);
> -  tree fndecl;
>    tree blck_size;
>    enum built_in_function fcode;
>    histogram_value histogram;
> @@ -1688,14 +1682,11 @@ gimple_stringops_transform (gimple_stmt_iterator *
>    tree tree_val;
>    int size_arg;
>
> -  if (gimple_code (stmt) != GIMPLE_CALL)
> +  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
>      return false;
> -  fndecl = gimple_call_fndecl (stmt);
> -  if (!fndecl)
> +
> +  if (!interesting_stringop_to_profile_p (stmt, &size_arg))
>      return false;
> -  fcode = DECL_FUNCTION_CODE (fndecl);
> -  if (!interesting_stringop_to_profile_p (fndecl, stmt, &size_arg))
> -    return false;
>
>    blck_size = gimple_call_arg (stmt, size_arg);
>    if (TREE_CODE (blck_size) == INTEGER_CST)
> @@ -1704,10 +1695,12 @@ gimple_stringops_transform (gimple_stmt_iterator *
>    histogram = gimple_histogram_value_of_type (cfun, stmt, HIST_TYPE_SINGLE_VALUE);
>    if (!histogram)
>      return false;
> +
>    val = histogram->hvalue.counters[0];
>    count = histogram->hvalue.counters[1];
>    all = histogram->hvalue.counters[2];
>    gimple_remove_histogram_value (cfun, stmt, histogram);
> +
>    /* We require that count is at least half of all; this means
>       that for the transformation to fire the value must be constant
>       at least 80% of time.  */
> @@ -1719,8 +1712,10 @@ gimple_stringops_transform (gimple_stmt_iterator *
>      prob = GCOV_COMPUTE_SCALE (count, all);
>    else
>      prob = 0;
> +
>    dest = gimple_call_arg (stmt, 0);
>    dest_align = get_pointer_alignment (dest);
> +  fcode = DECL_FUNCTION_CODE (gimple_call_fndecl (stmt));
>    switch (fcode)
>      {
>      case BUILT_IN_MEMCPY:
> @@ -1811,6 +1806,7 @@ stringop_block_profile (gimple stmt, unsigned int
>
>
>  /* Find values inside STMT for that we want to measure histograms for
>     division/modulo optimization.  */
> +
>  static void
>  gimple_divmod_values_to_profile (gimple stmt, histogram_values *values)
>  {
> @@ -1891,21 +1887,18 @@ gimple_indirect_call_to_profile (gimple stmt, hist
>
>  /* Find values inside STMT for that we want to measure histograms for
>     string operations.  */
> +
>  static void
>  gimple_stringops_values_to_profile (gimple stmt, histogram_values *values)
>  {
> -  tree fndecl;
> -  tree blck_size;
>    tree dest;
> +  tree blck_size;
>    int size_arg;
>
> -  if (gimple_code (stmt) != GIMPLE_CALL)
> +  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
>      return;
> -  fndecl = gimple_call_fndecl (stmt);
> -  if (!fndecl)
> -    return;
>
> -  if (!interesting_stringop_to_profile_p (fndecl, stmt, &size_arg))
> +  if (!interesting_stringop_to_profile_p (stmt, &size_arg))
>      return;
>
>    dest = gimple_call_arg (stmt, 0);
> @@ -1919,6 +1912,7 @@ gimple_stringops_values_to_profile (gimple stmt, h
>        values->safe_push (gimple_alloc_histogram_value (cfun, HIST_TYPE_AVERAGE,
>                                                        stmt, blck_size));
>      }
> +
>    if (TREE_CODE (blck_size) != INTEGER_CST)
>      values->safe_push (gimple_alloc_histogram_value (cfun, HIST_TYPE_IOR,
>                                                      stmt, dest));
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog       (revision 141081)
> +++ gcc/ChangeLog       (working copy)
> @@ -1,3 +1,12 @@
> +2014-08-20  Felix Yang  <felix.yang@huawei.com>
> +           Jiji Jiang  <jiangjiji@huawei.com>
> +
> +       * value-prof.c (interesting_stringop_to_profile_p): Removed FNDECL argument
> +       and get builtin function code directly from CALL.
> +       (gimple_stringop_fixed_value): Modified accordingly.
> +       (gimple_stringops_transform, gimple_stringops_values_to_profile): Modified
> +       accordingly and only accept BUILT_IN_NORMAL string operations.
> +
>  2015-08-18  Segher Boessenkool  <segher@kernel.crashing.org>
>
>         Backport from mainline:
>
>
>
>>
>> On Thu, Aug 20, 2015 at 5:17 AM, Yangfei (Felix) <felix.yang@huawei.com>
>> wrote:
>> > Hi,
>> >
>> >     As DECL_FUNCTION_CODE is overloaded for builtin functions in different
>> classes, so need to check builtin class before using fcode.
>> >     Patch posted below.  Bootstrapped on x86_64-suse-linux, OK for trunk?
>> >     Thanks.
>>
>> Ugh.  The code in the callers already looks like it could have some TLC, like
>> instead of
>>
>>   fndecl = gimple_call_fndecl (stmt);
>>   if (!fndecl)
>>     return false;
>>   fcode = DECL_FUNCTION_CODE (fndecl);
>>   if (!interesting_stringop_to_profile_p (fndecl, stmt, &size_arg))
>>     return false;
>>
>> simply do
>>
>>   if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
>>     return false;
>>   if (!interesting_stringop_to_profile_p (gimple_call_fndecl (stmt), ....))
>>
>> similar for the other caller.  interesting_stringop_to_profile_p can also get
>> function-code directly from stmt, removing the redundant first argument or even
>> do the gimple_call_builtin_p call itself.
>>
>> Mind reworking the patch accordingly?
>>
>> Thanks,
>> Richard.
>