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
