From patchwork Wed Jul 16 20:32:50 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: 370869 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 290F11400A3 for ; Thu, 17 Jul 2014 06:33:08 +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:date:message-id:subject:from:to:cc:content-type; q=dns; s=default; b=ICGpsHS0MKfi8vrXxNi6JwMYo66jz0b/R4EDxRdDOXD jtQNxJXcAiN2Ak6xshWWGKuOWVMNr1OiFrPg6rJZ+01BCr8df3YsxDKVnnVhgVPv c45WAo+RTr3/mIxXyjL+8mQMK6EoRPnKqxXAo9X7AbFKzSMo2hYtH6moy2zF5XcA = 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:date:message-id:subject:from:to:cc:content-type; s=default; bh=X3xelmBG6r3eKarbTy1LC0VTH10=; b=aCgPuKsGVODPFQwHe 2fI2LLqq+X5Ivj9xZXbkzb71h3puV7XwzKdxvx8eWCnyQ9GPeXFDFzZxm04z5lpY vmferCRqAOrG91B3QVS+SggySLL+LIPXU0mZhgG0VTG3Lt/UGoPyQlen2QMVGWZl 7gq0gJLVwgTxQOI+77mRi3/tDc= Received: (qmail 845 invoked by alias); 16 Jul 2014 20:33:00 -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 807 invoked by uid 89); 16 Jul 2014 20:32:58 -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-f49.google.com Received: from mail-wg0-f49.google.com (HELO mail-wg0-f49.google.com) (74.125.82.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 16 Jul 2014 20:32:54 +0000 Received: by mail-wg0-f49.google.com with SMTP id k14so1496588wgh.32 for ; Wed, 16 Jul 2014 13:32:50 -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:date:message-id:subject:from:to:cc :content-type; bh=5T1WX9mfwUitQSMfteWYl43TP6b2Hv7jqN3rTJl54t4=; b=kT/9YE14WyLcU8S+UUMkFqZhYLRFe4qybLh5NFfy9uvkS9oivF09PmhHZWHRvnwaKm zUt/vpWa8fMvF3hNpK4EkrItzvdPSTzf8f/W58e6TX1JyAsLYqwln46/OQkWYLF29h7J XN57bW/fHHmkuIadIZMt4GJkKFsserPxpSkmKYDRSP2dzZgpsVbjvMTCqtVcdcWAM1HP iql8Os2S1fJQlH1MQdlF++dHf5o7ePeH7Y3ojNiScCxrXiM/Vn5Z4JfvOdQUzrOEX746 UvvTdEVvml593OXnNZ4gBsRwfNtUa85VBEkOhWAOW5aUf15s/q2dN15c3Eo1j1CUKgLO XDYA== X-Gm-Message-State: ALoCoQn20ju0HyIgEWGyuRW2jVD2+wJ7nZuiJNsmN5Xo5qevPCCrdu6lWkkFnehgtcNM27vK/pdj MIME-Version: 1.0 X-Received: by 10.180.212.12 with SMTP id ng12mr16950920wic.9.1405542770644; Wed, 16 Jul 2014 13:32:50 -0700 (PDT) Received: by 10.180.7.34 with HTTP; Wed, 16 Jul 2014 13:32:50 -0700 (PDT) Date: Wed, 16 Jul 2014 13:32:50 -0700 Message-ID: Subject: FDO and source changes From: Xinliang David Li To: GCC Patches Cc: Jan Hubicka X-IsSubscribed: yes Instrumentation based FDO is designed to work when the source files that are used to generate the instr binary match exactly with the sources in profile-use compile. It is known historically that using stale profile (due to source changes, not gcda format change) can lead to lots of mismatch warnings and even worse -- compiler ICEs. This is due to two reasons: 1) the profile lookup for each function is based on funcdef_no which can change when function definition order is changed or new functions are inserted in the middle of a source 2) the indirect call target id may change due to source changes: before GCC4.9, the id uses cgraph uid which is as bad as funcdef_no. Attributing wrong IC target to the indirect call site is the main cause of compiler ICE (we have signature match check, but bad target can leak through result in problem later). Starting from gcc49, the indirect target profiling uses profile_id which is stable for public functions. This patch introduces a new parameter for FDO to determine whether to use internal id or assembler name based external id for profile lookup. When the external id is used, GCC FDO will become very tolerant to simple source changes. Note that autoFDO solves this problem but it is currently limited to Intel platforms with LBR support. (Tested with SPEC, SPEC06 and large internal benchmarks. No performance impact). Ok for trunk? thanks, David 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", + 1, 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/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: 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: 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,10 @@ 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 + elt.ident = coverage_compute_profile_id (cgraph_get_node (cfun->decl)); elt.ctr = counter; entry = counts_hash->find (&elt); if (!entry || !entry->summary.num) @@ -416,7 +419,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 +585,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 +650,12 @@ 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 + gcov_write_unsigned (coverage_compute_profile_id ( + cgraph_get_node (current_function_decl))); + gcov_write_unsigned (lineno_checksum); gcov_write_unsigned (cfg_checksum); gcov_write_string (IDENTIFIER_POINTER @@ -682,8 +692,13 @@ 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 + item->ident = coverage_compute_profile_id ( + cgraph_get_node (cfun->decl)); + item->lineno_checksum = lineno_checksum; item->cfg_checksum = cfg_checksum;