Re: [PATCH] tree-optimization/106912 - IPA profile and pure/const

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Mar 16, 2023 at 12:05:56PM +0000, Richard Biener wrote:
> On Thu, 16 Mar 2023, Jakub Jelinek wrote:
> 
> > On Fri, Nov 25, 2022 at 09:26:34PM +0100, Richard Biener via Gcc-patches wrote:
> > > > We could
> > > > probably keep tract if any instrumented code was ever inlined into a
> > > > given function and perhaps just start ignoring attributes set on types?
> > > 
> > > But ignoring attributes on types makes all indirect calls not
> > > const/pure annotatable (OK, they are not pure annotatable because of
> > > the above bug).  I really don't see how to conservatively solve this
> > > issue?
> > 
> > > Maybe we can ignore all pure/const when the cgraph state is
> > > in profile-instrumented state?  Of course we have multiple "APIs"
> > > to query that.
> > 
> > I think that's the way to go.  But we'd need to arrange somewhere during IPA
> > to add vops to all those pure/const calls if -fprofile-generate (direct or
> > indirect; not sure what exact flags) and after that make sure all our APIs
> > don't return ECF_CONST, ECF_PURE or ECF_LOOPING_CONST_OR_PURE.
> > Could be e.g.
> >   if (whatever)
> >     flags &= ~(ECF_CONST | ECF_PURE | ECF_LOOPING_CONST_OR_PURE);
> > at the end of flags_from_decl_or_type, internal_fn_flags, what else?
> > Although, perhaps internal_fn_flags don't need to change, because internal
> > calls don't really have callees.
> > 
> > Or is just ECF_CONST enough given that ECF_PURE is on fndecls and not
> > types?
> > 
> > Or perhaps just start ignoring TYPE_READONLY -> ECF_CONST in
> > flags_from_decl_or_type if that flag is on?
> > 
> > Is that rewriting currently what tree_profiling does in the
> >   /* Update call statements and rebuild the cgraph.  */
> >   FOR_EACH_DEFINED_FUNCTION (node)
> > spot where it calls update_stmt on all call statements?
> > 
> > If so, could we just set that global? flag instead or in addition to
> > doing those node->set_const_flag (false, false); calls and
> > change flags_from_decl_or_type, plus I guess lto1 should set that
> > flag if it is global on start as well if
> > !flag_auto_profile
> > && (flag_branch_probabilities || flag_test_coverage || profile_arc_flag)
> > ?
> > 
> > I think just ignoring ECF_CONST from TYPE_READONLY might be best, if we
> > have external functions like builtins from libc/libm and don't have their
> > bodies, we can still treat them as const, the only problem is with the
> > indirect calls where we don't know if we do or don't have a body for
> > the callees and whether we've instrumented those or not.
> 
> I think we want something reflected on the IL.  Because of LTO I think
> we cannot ignore externs (or we have to do massaging at stream-in).
> 
> The following brute-force works for the testcase.  I suppose since
> we leave const/pure set on functions without body in the compile-time
> TU (and ignore LTO there) we could do the same here.
> 
> I also wonder if that whole function walk is necessary if not
> profile_arc_flag || flag_test_coverage ...
> 
> Honza - does this look OK to you?  I'll test guarding the whole
> outer loop with profile_arc_flag || flag_test_coverage separately.
> 
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index ea9d7a23443..c8789618f96 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -840,9 +840,29 @@ tree_profiling (void)
>           gimple_stmt_iterator gsi;
>           for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>             {
> -             gimple *stmt = gsi_stmt (gsi);
> -             if (is_gimple_call (stmt))
> -               update_stmt (stmt);
> +             gcall *call = dyn_cast <gcall *> (gsi_stmt (gsi));
> +             if (!call)
> +               continue;
> +
> +             if (profile_arc_flag || flag_test_coverage)
> +               {
> +             /* We do not clear pure/const on decls without body.  */
> +             tree fndecl = gimple_call_fndecl (call);
> +             if (fndecl && !gimple_has_body_p (fndecl))
> +               continue;
> +
> +             /* Drop the const attribute from the call type (the pure
> +                attribute is not available on types).  */
> +             tree fntype = gimple_call_fntype (call);
> +             if (fntype && TYPE_READONLY (fntype))
> +               gimple_call_set_fntype (call, build_qualified_type
> +                                               (fntype, (TYPE_QUALS 
> (fntype)
> +                                                         & 
> ~TYPE_QUAL_CONST)));
> +               }
> +
> +             /* Update virtual operands of calls to no longer const/pure
> +                functions.  */
> +             update_stmt (call);
>             }
>         }

I have in the meantime briefly tested following.

But if you want to the above way, then at least the testcase could be
useful.  Though, not sure if the above is all that is needed.  Shouldn't
set_const_flag_1 upon TREE_READONLY (node->decl) = 0; also adjust
TREE_TYPE on the function to
	  tree fntype = TREE_TYPE (node->decl);
	  TREE_TYPE (node->decl)
	    = build_qualified_type (fntype,
				    TYPE_QUALS (fntype) & ~TYPE_QUAL_CONST);
too (perhaps guarded on TREE_READONLY (fntype))?

2023-03-16  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/106912
gcc/
	* calls.h (ignore_const_fntype): Declare.
	* calls.cc (flags_from_decl_or_type): Ignore TYPE_READONLY
	on types if ignore_const_fntype.
	* tree-profile.cc: Include calls.h.
	(tree_profiling): Set ignore_const_fntype if profile_arc_flag
	or flag_test_coverage.
gcc/lto/
	* lto-lang.cc (lto_post_options): Set ignore_const_fntype if
	profile_arc_flag or flag_test_coverage and not flag_auto_profile.
gcc/testsuite/
	* gcc.dg/pr106912.c: New test.

--- gcc/calls.h.jj	2023-01-02 09:32:51.252868185 +0100
+++ gcc/calls.h	2023-03-16 12:23:51.632460586 +0100
@@ -134,5 +134,6 @@ extern void maybe_complain_about_tail_ca
 
 extern rtx rtx_for_static_chain (const_tree, bool);
 extern bool cxx17_empty_base_field_p (const_tree);
+extern bool ignore_const_fntype;
 
 #endif // GCC_CALLS_H
--- gcc/calls.cc.jj	2023-02-21 11:44:48.460464845 +0100
+++ gcc/calls.cc	2023-03-16 12:27:45.427032110 +0100
@@ -800,6 +800,13 @@ is_tm_builtin (const_tree fndecl)
   return false;
 }
 
+/* Set if flags_from_decl_or_type should ignore TYPE_READONLY of function
+   types.  This is used when tree-profile.cc instruments const calls,
+   clears TREE_READONLY on FUNCTION_DECLs which have been instrumented, but
+   for function types or indirect calls we don't know if the callees have been
+   instrumented or not.  */
+bool ignore_const_fntype;
+
 /* Detect flags (function attributes) from the function decl or type node.  */
 
 int
@@ -849,7 +856,7 @@ flags_from_decl_or_type (const_tree exp)
     }
   else if (TYPE_P (exp))
     {
-      if (TYPE_READONLY (exp))
+      if (TYPE_READONLY (exp) && !ignore_const_fntype)
 	flags |= ECF_CONST;
 
       if (flag_tm
--- gcc/tree-profile.cc.jj	2023-01-02 09:32:27.923205268 +0100
+++ gcc/tree-profile.cc	2023-03-16 12:57:29.130873893 +0100
@@ -58,6 +58,7 @@ along with GCC; see the file COPYING3.
 #include "alloc-pool.h"
 #include "symbol-summary.h"
 #include "symtab-thunks.h"
+#include "calls.h"
 
 static GTY(()) tree gcov_type_node;
 static GTY(()) tree tree_interval_profiler_fn;
@@ -819,6 +820,10 @@ tree_profiling (void)
 	node->set_pure_flag (false, false);
       }
 
+  /* From this point on, ignore TYPE_READONLY on function types.  */
+  if (profile_arc_flag || flag_test_coverage)
+    ignore_const_fntype = true;
+
   /* Update call statements and rebuild the cgraph.  */
   FOR_EACH_DEFINED_FUNCTION (node)
     {
--- gcc/lto/lto-lang.cc.jj	2023-01-16 11:52:16.165732856 +0100
+++ gcc/lto/lto-lang.cc	2023-03-16 12:58:00.420414840 +0100
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.
 #include "lto-common.h"
 #include "stringpool.h"
 #include "attribs.h"
+#include "calls.h"
 
 /* LTO specific dumps.  */
 int lto_link_dump_id, decl_merge_dump_id, partition_dump_id;
@@ -938,6 +939,11 @@ lto_post_options (const char **pfilename
   if (!flag_merge_constants)
     flag_merge_constants = 1;
 
+  /* If const functions were or might have been instrumented by
+     tree-profiler, ignore TYPE_READONLY on function types.  */
+  if (!flag_auto_profile && (profile_arc_flag || flag_test_coverage))
+    ignore_const_fntype = true;
+
   /* Initialize the compiler back end.  */
   return false;
 }
--- gcc/testsuite/gcc.dg/pr106912.c.jj	2023-03-16 13:02:17.720639903 +0100
+++ gcc/testsuite/gcc.dg/pr106912.c	2023-03-16 13:03:47.472323118 +0100
@@ -0,0 +1,22 @@
+/* PR tree-optimization/106912 */
+/* { dg-do compile { target vect_simd_clones } } */
+/* { dg-require-profiling "-fprofile-generate" } */
+/* { dg-options "-O2 -ftree-vectorize -fprofile-generate" } */
+/* { dg-additional-options "-fPIC" { target fpic } } */
+
+__attribute__ ((__simd__, __nothrow__, __leaf__, __const__))
+double foo (double x);
+
+void
+bar (double *f, int n)
+{
+  int i;
+  for (i = 0; i < n; i++)
+    f[i] = foo (f[i]);
+}
+
+double
+foo (double x)
+{
+  return x * x / 3.0;
+}


	Jakub