On Wed, Jul 23, 2014 at 11:06 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>>
>> I don't think existing profile data matter.
>>
>> For perfect fresh profile, using external id has the chance of
>> collision. I have tested with a C++ symbol file with about 750k unique
>> symbol names, using crc32 based id yields 71 collisions --- the rate
>> is ~0.009%.
>
> init_node_map will resolve profile_id conflicts within single unit, so this is
> not causing any troubles (naturally the profile will read wose if one of the
> two conflicting symbols disappears, but that is an minor issue).
> So i think we want to go for profile_id by default.
Right -- I missed this part.
My original patch needs to be updated to use this feature:
1) instead of invoking coverage_compute_profile_id directly in several
places, use cgraph_node->profile_id which is conflict free
2) added assertion to make sure profile_id is set up before use
3) removed the restriction in cgraph_node_map computation which also
populate other functions not indirectly called
I also flip the default to use external id. The patch is attached.
No problems found in regression test. Ok for trunk after large app
testing with FDO?
thanks,
David
>
> Only what I am concerned about is to make C static functions that have same
> name in
> different translation units to not clash in profile_id or indirect call
> optimization
> will be behaving poorly. That is main reason why we mix in global symbol name.
>
> As mentioned in the original review, I think using gcov file name should be
> good enough...
>
> Honza
>>
>> >
>> > Given that we need both, why is this a param vs a regular -f option?
>> > Shouldn't the default be to use the external id?
>> >
>>
>> I am open to both. I have not seen evidence that id collision causes
>> trouble even though in theory it can.
>>
>> thanks,
>>
>> David
>>
>>
>> > BTW, thanks for working on this. I've certainly got customers that want to
>> > see the FDO data be more tolerant of changes.
>> >
>> > Heff
>> >
Index: params.def
===================================================================
--- params.def (revision 212682)
+++ params.def (working copy)
@@ -869,6 +869,14 @@ DEFPARAM (PARAM_LOOP_INVARIANT_MAX_BBS_I
"Max basic blocks number in loop for loop invariant motion",
10000, 0, 0)
+/* When the parameter is 1, use the internal function id
+ to look up for profile data. Otherwise, use a more stable
+ external id based on assembler name and source location. */
+DEFPARAM (PARAM_PROFILE_FUNC_INTERNAL_ID,
+ "profile-func-internal-id",
+ "use internal function id in profile lookup",
+ 0, 0, 1)
+
/* Avoid SLP vectorization of large basic blocks. */
DEFPARAM (PARAM_SLP_MAX_INSNS_IN_BB,
"slp-max-insns-in-bb",
Index: ChangeLog
===================================================================
--- ChangeLog (revision 212682)
+++ ChangeLog (working copy)
@@ -1,3 +1,10 @@
+2014-07-16 Xinliang David Li <davi...@google.com>
+
+ * params.def: New parameter.
+ * coverage.c (get_coverage_counts): Check new flag.
+ (coverage_compute_profile_id): Check new flag.
+ (coverage_begin_function): Check new flag.
+
2014-07-16 Dodji Seketeli <do...@redhat.com>
Support location tracking for built-in macro tokens
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog (revision 212682)
+++ testsuite/ChangeLog (working copy)
@@ -1,3 +1,11 @@
+2014-07-16 Xinliang David Li <davi...@google.com>
+
+ * g++.dg/tree-prof/tree-prof.exp: Define macros.
+ * g++.dg/tree-prof/reorder_class1.h: New file.
+ * g++.dg/tree-prof/reorder_class2.h: New file.
+ * g++.dg/tree-prof/reorder.C: New test.
+ * g++.dg/tree-prof/morefunc.C: New test.
+
2014-07-16 Arnaud Charlet <char...@adacore.com>
* gnat.db/specs/alignment2.ads, gnat.db/specs/size_clause1.ads,
Index: testsuite/g++.dg/tree-prof/reorder.C
===================================================================
--- testsuite/g++.dg/tree-prof/reorder.C (revision 0)
+++ testsuite/g++.dg/tree-prof/reorder.C (revision 0)
@@ -0,0 +1,48 @@
+/* { dg-options "-O2 -fno-devirtualize --param=profile-func-internal-id=0
-fdump-ipa-profile -Wno-coverage-mismatch -Wno-attributes" } */
+
+#ifdef _PROFILE_USE
+#include "reorder_class1.h"
+#include "reorder_class2.h"
+#else
+#include "reorder_class2.h"
+#include "reorder_class1.h"
+#endif
+
+int g;
+static __attribute__((always_inline))
+void test1 (A *tc)
+{
+ int i;
+ for (i = 0; i < 1000; i++)
+ g += tc->foo();
+ if (g<100) g++;
+}
+
+static __attribute__((always_inline))
+void test2 (B *tc)
+{
+ int i;
+ for (i = 0; i < 1000; i++)
+ g += tc->foo();
+}
+
+
+#ifdef _PROFILE_USE
+__attribute__((noinline)) void test_a(A *ap) { test1 (ap); }
+__attribute__((noinline)) void test_b(B *bp) { test2 (bp); }
+#else
+__attribute__((noinline)) void test_b(B *bp) { test2 (bp); }
+__attribute__((noinline)) void test_a(A *ap) { test1 (ap); }
+#endif
+
+int main()
+{
+ A* ap = new A();
+ B* bp = new B();
+
+ test_a(ap);
+ test_b(bp);
+}
+
+/* { dg-final-use { scan-ipa-dump-times "Indirect call -> direct call" 2
"profile" } } */
+
Index: testsuite/g++.dg/tree-prof/tree-prof.exp
===================================================================
--- testsuite/g++.dg/tree-prof/tree-prof.exp (revision 212682)
+++ testsuite/g++.dg/tree-prof/tree-prof.exp (working copy)
@@ -42,8 +42,8 @@ set PROFOPT_OPTIONS [list {}]
# These are globals used by profopt-execute. The first is options
# needed to generate profile data, the second is options to use the
# profile data.
-set profile_option "-fprofile-generate"
-set feedback_option "-fprofile-use"
+set profile_option "-fprofile-generate -D_PROFILE_GENERATE"
+set feedback_option "-fprofile-use -D_PROFILE_USE"
foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.C]] {
# If we're only testing specific files and this isn't one of them, skip it.
Index: testsuite/g++.dg/tree-prof/reorder_class1.h
===================================================================
--- testsuite/g++.dg/tree-prof/reorder_class1.h (revision 0)
+++ testsuite/g++.dg/tree-prof/reorder_class1.h (revision 0)
@@ -0,0 +1,11 @@
+struct A {
+ virtual int foo();
+};
+
+int A::foo()
+{
+ return 1;
+}
+
+
+
Index: testsuite/g++.dg/tree-prof/reorder_class2.h
===================================================================
--- testsuite/g++.dg/tree-prof/reorder_class2.h (revision 0)
+++ testsuite/g++.dg/tree-prof/reorder_class2.h (revision 0)
@@ -0,0 +1,12 @@
+
+struct B {
+ virtual int foo();
+};
+
+int B::foo()
+{
+ return 2;
+}
+
+
+
Index: testsuite/g++.dg/tree-prof/morefunc.C
===================================================================
--- testsuite/g++.dg/tree-prof/morefunc.C (revision 0)
+++ testsuite/g++.dg/tree-prof/morefunc.C (revision 0)
@@ -0,0 +1,55 @@
+/* { dg-options "-O2 -fno-devirtualize --param=profile-func-internal-id=0
-fdump-ipa-profile -Wno-attributes -Wno-coverage-mismatch" } */
+#include "reorder_class1.h"
+#include "reorder_class2.h"
+
+int g;
+
+#ifdef _PROFILE_USE
+/* Another function not existing
+ * in profile-gen */
+
+__attribute__((noinline)) void
+new_func (int i)
+{
+ g += i;
+}
+#endif
+
+static __attribute__((always_inline))
+void test1 (A *tc)
+{
+ int i;
+ for (i = 0; i < 1000; i++)
+ g += tc->foo();
+ if (g<100) g++;
+}
+
+static __attribute__((always_inline))
+void test2 (B *tc)
+{
+ int i;
+ for (i = 0; i < 1000; i++)
+ g += tc->foo();
+}
+
+
+__attribute__((noinline)) void test_a(A *ap) { test1 (ap); }
+__attribute__((noinline)) void test_b(B *bp) { test2 (bp); }
+
+
+int main()
+{
+ A* ap = new A();
+ B* bp = new B();
+
+ test_a(ap);
+ test_b(bp);
+
+#ifdef _PROFILE_USE
+ new_func(10);
+#endif
+
+}
+
+/* { dg-final-use { scan-ipa-dump-times "Indirect call -> direct call" 2
"profile" } } */
+
Index: value-prof.c
===================================================================
--- value-prof.c (revision 212682)
+++ value-prof.c (working copy)
@@ -1210,7 +1210,17 @@ gimple_mod_subtract_transform (gimple_st
return true;
}
-static pointer_map_t *cgraph_node_map;
+static pointer_map_t *cgraph_node_map = 0;
+
+/* Returns true if node graph is initialized. This
+ is used to test if profile_id has been created
+ for cgraph_nodes. */
+
+bool
+coverage_node_map_initialized_p (void)
+{
+ return cgraph_node_map != 0;
+}
/* Initialize map from PROFILE_ID to CGRAPH_NODE.
When LOCAL is true, the PROFILE_IDs are computed. when it is false we
assume
@@ -1223,8 +1233,7 @@ init_node_map (bool local)
cgraph_node_map = pointer_map_create ();
FOR_EACH_DEFINED_FUNCTION (n)
- if (cgraph_function_with_gimple_body_p (n)
- && !cgraph_only_called_directly_p (n))
+ if (cgraph_function_with_gimple_body_p (n))
{
void **val;
if (local)
Index: coverage.c
===================================================================
--- coverage.c (revision 212682)
+++ coverage.c (working copy)
@@ -54,6 +54,7 @@ along with GCC; see the file COPYING3.
#include "intl.h"
#include "filenames.h"
#include "target.h"
+#include "params.h"
#include "gcov-io.h"
#include "gcov-io.c"
@@ -369,8 +370,13 @@ get_coverage_counts (unsigned counter, u
da_file_name);
return NULL;
}
-
- elt.ident = current_function_funcdef_no + 1;
+ if (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID))
+ elt.ident = current_function_funcdef_no + 1;
+ else
+ {
+ gcc_assert (coverage_node_map_initialized_p ());
+ elt.ident = cgraph_get_node (cfun->decl)->profile_id;
+ }
elt.ctr = counter;
entry = counts_hash->find (&elt);
if (!entry || !entry->summary.num)
@@ -416,7 +422,8 @@ get_coverage_counts (unsigned counter, u
}
else if (entry->lineno_checksum != lineno_checksum)
{
- warning (0, "source locations for function %qE have changed,"
+ warning (OPT_Wcoverage_mismatch,
+ "source locations for function %qE have changed,"
" the profile data may be out of date",
DECL_ASSEMBLER_NAME (current_function_decl));
}
@@ -581,12 +588,13 @@ coverage_compute_profile_id (struct cgra
{
expanded_location xloc
= expand_location (DECL_SOURCE_LOCATION (n->decl));
+ bool use_name_only = (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID) == 0);
- chksum = xloc.line;
+ chksum = (use_name_only ? 0 : xloc.line);
chksum = coverage_checksum_string (chksum, xloc.file);
chksum = coverage_checksum_string
(chksum, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (n->decl)));
- if (first_global_object_name)
+ if (!use_name_only && first_global_object_name)
chksum = coverage_checksum_string
(chksum, first_global_object_name);
chksum = coverage_checksum_string
@@ -645,7 +653,15 @@ coverage_begin_function (unsigned lineno
/* Announce function */
offset = gcov_write_tag (GCOV_TAG_FUNCTION);
- gcov_write_unsigned (current_function_funcdef_no + 1);
+ if (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID))
+ gcov_write_unsigned (current_function_funcdef_no + 1);
+ else
+ {
+ gcc_assert (coverage_node_map_initialized_p ());
+ gcov_write_unsigned (
+ cgraph_get_node (current_function_decl)->profile_id);
+ }
+
gcov_write_unsigned (lineno_checksum);
gcov_write_unsigned (cfg_checksum);
gcov_write_string (IDENTIFIER_POINTER
@@ -682,8 +698,15 @@ coverage_end_function (unsigned lineno_c
if (!DECL_EXTERNAL (current_function_decl))
{
item = ggc_alloc<coverage_data> ();
-
- item->ident = current_function_funcdef_no + 1;
+
+ if (PARAM_VALUE (PARAM_PROFILE_FUNC_INTERNAL_ID))
+ item->ident = current_function_funcdef_no + 1;
+ else
+ {
+ gcc_assert (coverage_node_map_initialized_p ());
+ item->ident = cgraph_get_node (cfun->decl)->profile_id;
+ }
+
item->lineno_checksum = lineno_checksum;
item->cfg_checksum = cfg_checksum;
Index: coverage.h
===================================================================
--- coverage.h (revision 212682)
+++ coverage.h (working copy)
@@ -56,5 +56,6 @@ extern gcov_type *get_coverage_counts (u
const struct gcov_ctr_summary **);
extern tree get_gcov_type (void);
+extern bool coverage_node_map_initialized_p (void);
#endif
