From patchwork Wed Jul 23 21:52:39 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xinliang David Li X-Patchwork-Id: 373066 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id AB7A4140186 for ; Thu, 24 Jul 2014 07:52:52 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=G8nBQmhNFoOHjuHOb9 4q9jnWUrEBCZ8jH1BxhVaZs6qO8dpbZtk+EDWeMM+Li5HxkQXF0pLoLCxUs0M2tg pvLy3XRCuCjLyKApgXepNuLtt8z0moEYaa2hOSEb3e8iwBsf8apt+J6tnXnhCsqv n+7cyBsh2+gpuR9TqROB+lIio= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=gGUZiQn/QH4RMr9eKWtXr7G8 43M=; b=HjQzxLvAVbfqDl+o1SGgeGGFxzwgfSNYrMfKrYAUwRsdTIrMV/5ardMK xiyPB4GQPV+Lo53aYIGQtjj5feRfYOtmdT0w1PKqLU6C5MXYz9dKWFiIm8FgYaJO 0oHL19MLimWziDxSCELYxJrrGDHdYIP9pc6wDsF8HncFaU6h8l0= Received: (qmail 29160 invoked by alias); 23 Jul 2014 21:52:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 29149 invoked by uid 89); 23 Jul 2014 21:52:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f50.google.com Received: from mail-wg0-f50.google.com (HELO mail-wg0-f50.google.com) (74.125.82.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 23 Jul 2014 21:52:42 +0000 Received: by mail-wg0-f50.google.com with SMTP id n12so1862272wgh.9 for ; Wed, 23 Jul 2014 14:52:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=W8VcrtJWvK7ONyEG5CfDGK034eZplSjYumjpOKqgjc8=; b=Bu3lOPn8fFp2QJOmYk9vDVyiL0pPB/pZvEyUj133pSfvds6sVBgn3Ws2i9gCbye/GW 1UdzL2e1LcXQcRTf6y/CRbFEyAIq8oJcbs1pshKagoDdSBxUk/dmMpXx2LxtVUPNivNh hBmEAO2dF+XU+Dr3VCfSRNCZiAMB2esA/5N9eVAPY/4NGp5hjb2+JhtnmfrkIiSreHKa srq08sSWkyVOZZi5LLMoVVDjiFUvNwFab3xK+y0DUW/yHOchuh3CFVNAASJHqxMibGR6 eOEXbvn3Z40fggyH0zBWNjd2EXmocGt+yEK5tx5tLDon2EyRuV9glhj8E2yaSxs+D+N4 fmWA== X-Gm-Message-State: ALoCoQkvKZEewq79nMwXLIXWR9X3qzo4sKwRdbbTXuTMS70a2Qr31zWKm+41QRiYjwMWpDhWnJ0Q MIME-Version: 1.0 X-Received: by 10.180.75.230 with SMTP id f6mr2428605wiw.5.1406152359524; Wed, 23 Jul 2014 14:52:39 -0700 (PDT) Received: by 10.180.7.34 with HTTP; Wed, 23 Jul 2014 14:52:39 -0700 (PDT) In-Reply-To: <20140723180614.GC21250@kam.mff.cuni.cz> References: <53CF2896.1060300@redhat.com> <20140723180614.GC21250@kam.mff.cuni.cz> Date: Wed, 23 Jul 2014 14:52:39 -0700 Message-ID: Subject: Re: FDO and source changes From: Xinliang David Li To: Jan Hubicka Cc: Jeff Law , GCC Patches X-IsSubscribed: yes On Wed, Jul 23, 2014 at 11:06 AM, Jan Hubicka 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 + + * 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 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 + + * 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 * 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 (); - - 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