From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68975 invoked by alias); 9 Dec 2015 12:16:04 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 68965 invoked by uid 89); 9 Dec 2015 12:16:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 09 Dec 2015 12:16:02 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id ED12E67C34 for ; Wed, 9 Dec 2015 12:16:00 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-113-142.phx2.redhat.com [10.3.113.142]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tB9CFwiG006159 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Wed, 9 Dec 2015 07:15:59 -0500 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id tB9CFuEM011124 for ; Wed, 9 Dec 2015 13:15:57 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id tB9CFtbh011123 for gcc-patches@gcc.gnu.org; Wed, 9 Dec 2015 13:15:55 +0100 Date: Wed, 09 Dec 2015 12:16:00 -0000 From: Jakub Jelinek To: GCC Patches Subject: Re: [hsa 3/10] HSA libgomp plugin Message-ID: <20151209121555.GQ5675@tucnak.redhat.com> Reply-To: Jakub Jelinek References: <20151207111758.GA24234@virgil.suse.cz> <20151207112049.GD24234@virgil.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151207112049.GD24234@virgil.suse.cz> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg00985.txt.bz2 On Mon, Dec 07, 2015 at 12:20:49PM +0100, Martin Jambor wrote: > +#include > +#include > +#include > +#include > +#include "libgomp-plugin.h" > +#include "gomp-constants.h" > +#include "hsa.h" > +#include "hsa_ext_finalize.h" If these 2 headers are coming from outside of gcc, better use <> for them instead of "", and better place them above the libgomp specific ones. > +#include "dlfcn.h" Ditto. > +/* Flag to decide whether print to stderr information about what is going on. > + Set in init_debug depending on environment variables. */ > + > +static bool debug; > + > +/* Flag to decide if the runtime should suppress a possible fallback to host > + execution. */ > + > +static bool suppress_host_fallback; > + > +/* Initialize debug and suppress_host_fallback according to the environment. */ > + > +static void > +init_enviroment_variables (void) > +{ > + if (getenv ("HSA_DEBUG")) > + debug = true; > + else > + debug = false; > + > + if (getenv ("HSA_SUPPRESS_HOST_FALLBACK")) > + suppress_host_fallback = true; > + else > + suppress_host_fallback = false; Wonder whether we want these custom env vars in suid programs, allowing users to change behavior might be undesirable. So perhaps use secure_getenv instead of getenv if found by configure? > + > + const char* hsa_error; Space before * instead of after it (multiple spots). > + hsa_status_string (status, &hsa_error); > + > + unsigned l = strlen (hsa_error); > + > + char *err = GOMP_PLUGIN_malloc (sizeof (char) * l); sizeof (char) is 1, please don't multiply by it, that is just confusing. It makes sense in macros where you don't know what the type really is, but not here. > + memcpy (err, hsa_error, l - 1); > + err[l] = '\0'; > + > + fprintf (stderr, "HSA warning: %s (%s)\n", str, err); Why do you allocate err at all, if you free it right away? Can't you use hsa_error directly instead? > + > + free (err); > +static void > +hsa_fatal (const char *str, hsa_status_t status) > +{ > + const char* hsa_error; > + hsa_status_string (status, &hsa_error); > + GOMP_PLUGIN_fatal ("HSA fatal error: %s (%s)", str, hsa_error); Like you do e.g. here? > +/* Callback of dispatch queues to report errors. */ > + > +static void > +queue_callback(hsa_status_t status, hsa_queue_t* queue __attribute__ ((unused)), Missing space before (. > +/* Callback of hsa_agent_iterate_regions. Determine if a memory REGION can be > + used for kernarg allocations and if so write it to the memory pointed to by > + DATA and break the query. */ > + > +static hsa_status_t get_kernarg_memory_region (hsa_region_t region, void* data) Missing newline between return type and function name. > + hsa_region_t* ret = (hsa_region_t*) data; Two spots with wrong formatting above. > +{ > + if (agent->first_module) > + agent->first_module->prev = module; Wrong indentation. > + unsigned size = end - start; > + char *buf = (char *) malloc (size); > + memcpy (buf, start, size); malloc could return NULL and you crash. Should this use GOMP_PLUGIN_malloc instead? > + struct GOMP_hsa_kernel_dispatch *shadow = GOMP_PLUGIN_malloc_cleared > + (sizeof (struct GOMP_hsa_kernel_dispatch)); Formatting, = should go on the next line, and if even that doesn't help, maybe use sizeof (*shadow)? > + > + shadow->queue = agent->command_q; > + shadow->omp_data_memory = omp_data_size > 0 > + ? GOMP_PLUGIN_malloc (omp_data_size) : NULL; = should go on the next line. > + unsigned dispatch_count = kernel->dependencies_count; > + shadow->kernel_dispatch_count = dispatch_count; > + > + shadow->children_dispatches = GOMP_PLUGIN_malloc > + (dispatch_count * sizeof (struct GOMP_hsa_kernel_dispatch *)); Likewise. > +indent_stream (FILE *f, unsigned indent) > +{ > + for (int i = 0; i < indent; i++) > + fputc (' ', f); Perhaps fprintf (f, "%*s", indent, ""); instead? > + struct GOMP_hsa_kernel_dispatch *shadow = create_single_kernel_dispatch > + (kernel, omp_data_size); Formatting issues again. > + shadow->omp_num_threads = 64; > + shadow->debug = 0; > + shadow->omp_level = kernel->gridified_kernel_p ? 1 : 0; > + > + /* Create kernel dispatch data structures. We do not allow to have > + a kernel dispatch with depth bigger than one. */ > + for (unsigned i = 0; i < kernel->dependencies_count; i++) > + { > + struct kernel_info *dependency = get_kernel_for_agent > + (kernel->agent, kernel->dependencies[i]); > + shadow->children_dispatches[i] = create_single_kernel_dispatch > + (dependency, omp_data_size); > + shadow->children_dispatches[i]->queue = Never put = at the end of line. > + kernel->agent->kernel_dispatch_command_q; Jakub