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);
}
}
