From patchwork Mon Oct 3 12:13:45 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Martin_Li=C5=A1ka?= X-Patchwork-Id: 677704 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 3sngvP1gKmz9s5g for ; Mon, 3 Oct 2016 23:14:07 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=ZWsTMd0u; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=iP08XUMpq3WGy2G2V GImvj4uw7eFFDXXb8ptEvst04ajrejlCN2nYIjRczhHqN8z62GVAY4ngrtlKjvGM n30WaKQ7+bJdOkjjWZIoTJ6BK8Eedyq+6NzMp4wV9SuhSDBYON8URkW892t5r/XJ MdxmxbwuSoHqUkH0RDySDXAjnw= 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 :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=XFVwXq2vwsURfNcXWKMFnb4 KsD8=; b=ZWsTMd0uN8SwiVmwEwZUsJNg6BLOLK1aaXXTuVoIHZaL0AVzVV2EBCt IMEq8KU9ETRvWYY85KoLDlsYSbwZqpX41fhz8WUaaJnelvonuMbPhhK2nsOx62HK LZ55sofCLdT92kXsTQh/HLCZjDVK45nY+7mSXO3BE48uiEt8VM+g= Received: (qmail 35840 invoked by alias); 3 Oct 2016 12:13:58 -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 35827 invoked by uid 89); 3 Oct 2016 12:13:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_PASS autolearn=ham version=3.3.2 spammy=odds X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Oct 2016 12:13:48 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9C2A5AC9E; Mon, 3 Oct 2016 12:13:45 +0000 (UTC) Subject: Re: [PATCH] Set -fprofile-update=atomic when -pthread is present To: Jeff Law , Andi Kleen References: <87f2bc4f-c4df-eadd-aec6-a937ed0ccaba@acm.org> <1253ac69-3301-f185-e43a-a34cadf8f51e@suse.cz> <67fda6d2-9b3e-a0d1-effc-34e1115030b2@acm.org> <1ff3cc75-7cee-79f3-395b-ef7a4d286a3d@acm.org> <04a05835-4666-4d7d-c1a9-d4bcc4ea924a@suse.cz> <87k2fpdatl.fsf@tassilo.jf.intel.com> <6f8b1905-818b-bfff-1bf3-5ba04f3b4b64@suse.cz> <20160818155130.GE5871@two.firstfloor.org> <631cf1bd-8ae9-f07b-5672-5084b699f650@redhat.com> Cc: Nathan Sidwell , gcc-patches@gcc.gnu.org, jh@suse.cz From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <2c750f4c-c96c-2b22-43ae-53bbebf18af8@suse.cz> Date: Mon, 3 Oct 2016 14:13:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2 MIME-Version: 1.0 In-Reply-To: <631cf1bd-8ae9-f07b-5672-5084b699f650@redhat.com> X-IsSubscribed: yes On 08/18/2016 05:53 PM, Jeff Law wrote: > On 08/18/2016 09:51 AM, Andi Kleen wrote: >>> I'd prefer to make updates atomic in multi-threaded applications. >>> The best proxy we have for that is -pthread. >>> >>> Is it slower, most definitely, but odds are we're giving folks >>> garbage data otherwise, which in many ways is even worse. >> >> It will likely be catastrophically slower in some cases. >> >> Catastrophically as in too slow to be usable. >> >> An atomic instruction is a lot more expensive than a single increment. Also >> they sometimes are really slow depending on the state of the machine. > And for those cases there's a way to override. > > The default should be set for correctness. > > jeff I would to somehow resolve the discussion related to default value selection. Is the prevailing consensus that we should set -fprofile-update=atomic when -pthread is set? If so, I'll prepare a patch. I tend to do it this way. Moreover, I also have a patch that provides a warning, which can be also useful even though we would change the default behavior: $ ./xgcc -B. /tmp/a.c -fprofile-update=single -pthread -fprofile-generate xgcc: warning: -profile-update=atomic should be used to generate a valid profile for a multithreaded application Ideas? Martin From d5a8097dd07d1a3f4263da7ccad970543d92f3e9 Mon Sep 17 00:00:00 2001 From: marxin Date: Mon, 3 Oct 2016 14:02:14 +0200 Subject: [PATCH] Warn about -fprofile-update=single and -pthread gcc/ChangeLog: 2016-10-03 Martin Liska * common.opt: Mark couple of flags with 'Driver' keyword. * gcc.c (driver_handle_option): Handle these options. (process_command): Generate the warning. --- gcc/common.opt | 8 ++++---- gcc/gcc.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/gcc/common.opt b/gcc/common.opt index 0e01577..3af9c64 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1920,7 +1920,7 @@ Common Report Var(profile_flag) Enable basic program profiling code. fprofile-arcs -Common Report Var(profile_arc_flag) +Common Driver Report Var(profile_arc_flag) Insert arc-based program profiling code. fprofile-dir= @@ -1933,7 +1933,7 @@ Common Report Var(flag_profile_correction) Enable correction of flow inconsistent profile data input. fprofile-update= -Common Joined RejectNegative Enum(profile_update) Var(flag_profile_update) Init(PROFILE_UPDATE_SINGLE) +Common Driver Joined RejectNegative Enum(profile_update) Var(flag_profile_update) Init(PROFILE_UPDATE_SINGLE) -fprofile-update=[single|atomic] Set the profile update method. Enum @@ -1946,11 +1946,11 @@ EnumValue Enum(profile_update) String(atomic) Value(PROFILE_UPDATE_ATOMIC) fprofile-generate -Common +Common Driver Enable common options for generating profile info for profile feedback directed optimizations. fprofile-generate= -Common Joined RejectNegative +Common Driver Joined RejectNegative Enable common options for generating profile info for profile feedback directed optimizations, and set -fprofile-dir=. fprofile-use diff --git a/gcc/gcc.c b/gcc/gcc.c index d3e8c88..b023013 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -233,6 +233,16 @@ static int print_subprocess_help; /* Linker suffix passed to -fuse-ld=... */ static const char *use_ld; +/* Flag indicating whether pthread is provided as a command line option. */ +static bool pthread_set = false; + +/* Flag indicating whether profiling is enabled by an option */ +static bool profiling_enabled = false; + +/* Flag indicating whether profile-update=atomic is provided as a command + line option. */ +static bool profile_update_atomic = false; + /* Whether we should report subprocess execution times to a file. */ FILE *report_times_to_file = NULL; @@ -4112,6 +4122,22 @@ driver_handle_option (struct gcc_options *opts, handle_foffload_option (arg); break; + case OPT_fprofile_update_: + if ((profile_update)value == PROFILE_UPDATE_ATOMIC) + profile_update_atomic = true; + break; + + case OPT_pthread: + pthread_set = true; + break; + + case OPT_fprofile_generate: + case OPT_fprofile_generate_: + case OPT_fprofile_arcs: + case OPT_coverage: + profiling_enabled = true; + break; + default: /* Various driver options need no special processing at this point, having been handled in a prescan above or being @@ -4580,6 +4606,11 @@ process_command (unsigned int decoded_options_count, add_infile ("help-dummy", "c"); } + /* Warn about multi-threaded program that do not use -profile=atomic. */ + if (profiling_enabled && pthread_set && !profile_update_atomic) + warning (0, "-profile-update=atomic should be used to generate a valid" + " profile for a multithreaded application"); + /* Decide if undefined variable references are allowed in specs. */ /* --version and --help alone or together are safe. Note that -v would -- 2.9.2