[hsa,3/10] HSA libgomp plugin [part 2/2]

On 12/09/2015 01:15 PM, Jakub Jelinek wrote:
> On Mon, Dec 07, 2015 at 12:20:49PM +0100, Martin Jambor wrote:
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <pthread.h>
>> +#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
> 

Hi.

The second part introduces usage of 'secure_getenv' routine.

Martin