From patchwork Thu Dec 10 17:52:07 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 555228 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 3274D140297 for ; Fri, 11 Dec 2015 04:52:21 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=Ge4ydQr1; 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:date :from:to:subject:message-id:references:mime-version:content-type :in-reply-to; q=dns; s=default; b=cDwTWg1wdhtqr2sq6KYdbo/C1cUj0d XxREjJG82QxSdTOmxK8qSq65RFMZB7UtZnnyWH+7iGZ43BhjJBh5j8qkbcjNPmIZ ymklzG0DReeAga3xROZiv7H5JoG5PmzmgchR/DUUYwf3ubbSOwUbmuUiIU8/rRzp GaXYDVSrUJFWg= 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:date :from:to:subject:message-id:references:mime-version:content-type :in-reply-to; s=default; bh=uY9x4unPQgCY9Q+Rkx9L81wzp+M=; b=Ge4y dQr1fyaBzPUIT8Ac6riRZONCecfeT0ZGXtLvcZtjxXVSq/VQfKMVHImmQ1sxTkBl px/4W/cbIw3F5U2E5rwfJFq8ts/2Ftl+zRkEtXZuWOzdzzTOoOlLadlMmmkn4SNz N9SWRVbSivl/CcKUphMwLnQO4hEN9x04BK+/Hxs= Received: (qmail 53950 invoked by alias); 10 Dec 2015 17:52:12 -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 53930 invoked by uid 89); 10 Dec 2015 17:52:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 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 (CAMELLIA256-SHA encrypted) ESMTPS; Thu, 10 Dec 2015 17:52:10 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 779CFAAB4; Thu, 10 Dec 2015 17:52:07 +0000 (UTC) Date: Thu, 10 Dec 2015 18:52:07 +0100 From: Martin Jambor To: GCC Patches , Jakub Jelinek , rdsandiford@googlemail.com Subject: Re: [hsa 1/10] Configury changes and new options Message-ID: <20151210175207.GF3534@virgil.suse.cz> Mail-Followup-To: GCC Patches , Jakub Jelinek , rdsandiford@googlemail.com References: <20151207111758.GA24234@virgil.suse.cz> <20151207111908.GB24234@virgil.suse.cz> <87vb88imt8.fsf@googlemail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87vb88imt8.fsf@googlemail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes Hi, On Tue, Dec 08, 2015 at 10:43:15PM +0000, Richard Sandiford wrote: > [Sorry for the low-quality review, was just reading out of interest...] > > Martin Jambor writes: > > +If you configure GCC with HSA offloading but do not have the HSA > > +run-time library installed in a standard location then you can > > +explicitely specify the directory where they are installed. The > > typo: explicitly oops. For some reason, my spell-checker accepts this typo. I will fix it. > > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c > > index e4772d1..5609207 100644 > > --- a/gcc/lto-wrapper.c > > +++ b/gcc/lto-wrapper.c > > @@ -745,6 +745,11 @@ compile_images_for_offload_targets (unsigned in_argc, char *in_argv[], > > offload_names = XCNEWVEC (char *, num_targets + 1); > > for (unsigned i = 0; i < num_targets; i++) > > { > > + /* HSA does not use LTO-like streaming and a different compiler, skip > > + it. */ > > + if (strncmp(names[i], "hsa", 3) == 0) > > + continue; > > + > > offload_names[i] > > = compile_offload_image (names[i], compiler_path, in_argc, in_argv, > > compiler_opts, compiler_opt_count, > > Looks like this would cause the caller loop: > > if (offload_names) > { > find_offloadbeginend (); > for (i = 0; offload_names[i]; i++) > printf ("%s\n", offload_names[i]); > free_array_of_ptrs ((void **) offload_names, i); > } > > to terminate early if there was another target after hsa. > Good catch. I have modified this code so that it never leaves any holes in offload_names[i]. > names[i] is null-terminated, so it looks like you're deliberately > allowing anything that starts with "hsa" here, but: Right, and that was probably a mistake, I have changed the check to simple strcmp. > > > diff --git a/gcc/opts.c b/gcc/opts.c > > index 874c84f..5647f0c 100644 > > --- a/gcc/opts.c > > +++ b/gcc/opts.c > > @@ -1906,8 +1906,35 @@ common_handle_option (struct gcc_options *opts, > > break; > > > > case OPT_foffload_: > > - /* Deferred. */ > > - break; > > + { > > + const char *p = arg; > > + opts->x_flag_disable_hsa = true; > > + while (*p != 0) > > + { > > + const char *comma = strchr (p, ','); > > + > > + if ((strncmp (p, "disable", 7) == 0) > > + && (p[7] == ',' || p[7] == '\0')) > > + { > > + opts->x_flag_disable_hsa = true; > > + break; > > + } > > + > > + if ((strncmp (p, "hsa", 3) == 0) > > + && (p[3] == ',' || p[3] == '\0')) > > + { > > +#ifdef ENABLE_HSA > > + opts->x_flag_disable_hsa = false; > > +#else > > + sorry ("HSA has not been enabled during configuration"); > > +#endif > > ...here you only allow "hsa" itself. > > (Not your fault, but: do we have any documentation for -foffload > and -foffload-abi? Couldn't see any in the texi files.) Yes, that is actually PR 67300. However, I do not understand the more complex forms the parameter can take enough to attempt to fix it. In order to address all for you concerns, I am going to install the following on the branch. Thanks for the feedback, Martin 2015-12-09 Martin Jambor * lto-wrapper.c (compile_images_for_offload_targets): Do not leave holes in offload_names. Use strcmp instead strncmp. * doc/install.texi (--with-hsa-runtime): Fix typo. --- gcc/doc/install.texi | 2 +- gcc/lto-wrapper.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi index afd891c..a85a063 100644 --- a/gcc/doc/install.texi +++ b/gcc/doc/install.texi @@ -1993,7 +1993,7 @@ compiler will emit the accelerator code, no path should be specified. If you configure GCC with HSA offloading but do not have the HSA run-time library installed in a standard location then you can -explicitely specify the directory where they are installed. The +explicitly specify the directory where they are installed. The @option{--with-hsa-runtime=@/@var{hsainstalldir}} option is a shorthand for @option{--with-hsa-runtime-lib=@/@var{hsainstalldir}/lib} and diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 5609207..5b58fd6 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -736,6 +736,7 @@ compile_images_for_offload_targets (unsigned in_argc, char *in_argv[], return; unsigned num_targets = parse_env_var (target_names, &names, NULL); + int next_name_entry = 0; const char *compiler_path = getenv ("COMPILER_PATH"); if (!compiler_path) goto out; @@ -747,16 +748,17 @@ compile_images_for_offload_targets (unsigned in_argc, char *in_argv[], { /* HSA does not use LTO-like streaming and a different compiler, skip it. */ - if (strncmp(names[i], "hsa", 3) == 0) + if (strcmp (names[i], "hsa") == 0) continue; - offload_names[i] + offload_names[next_name_entry] = compile_offload_image (names[i], compiler_path, in_argc, in_argv, compiler_opts, compiler_opt_count, linker_opts, linker_opt_count); - if (!offload_names[i]) + if (!offload_names[next_name_entry]) fatal_error (input_location, "problem with building target image for %s\n", names[i]); + next_name_entry++; } out: