On Wed, Jul 01, 2020 at 10:50:44PM -0400, y2s1982 . via Gcc wrote:
> per-process functions defined in 5.5.2.
> I have some questions on defining or at least using some of the data types.
The standard makes those intentionally not defined, it is up to the
implementation to put in there whatever is needed/useful. That is the whole
idea behind handles, which are pointer to these, but it is not ok for the
debugger to make assumptions on what exactly it points to
(similarly how e.g. with the context handles which are again pointers to
some data in the debugger and OMPD shouldn't assume anything, just pass it
around).
> --- a/libgomp/libgompd.h
> +++ b/libgomp/libgompd.h
> @@ -29,9 +29,13 @@
> #ifndef LIBGOMPD_H
> #define LIBGOMPD_H 1
>
> +#include "omp-tools.h"
> +
> #define ompd_stringify(x) ompd_str2(x)
> #define ompd_str2(x) #x
>
> #define OMPD_VERSION 201811
>
> +extern ompd_callbacks_t gompd_callbacks;
> +
Confused, weren't these changes already in the previous patch?
(and other hunks too).
> #endif /* LIBGOMPD_H */
> diff --git a/libgomp/ompd-lib.c b/libgomp/ompd-lib.c
> index f0ae9e85a7e..d5350e1045c 100644
> --- a/libgomp/ompd-lib.c
> +++ b/libgomp/ompd-lib.c
> @@ -29,6 +29,9 @@
> #include "omp-tools.h"
> #include "libgompd.h"
>
> +ompd_callbacks_t gompd_callbacks;
> +static int ompd_initialized = 0;
> +
> ompd_rc_t
> ompd_get_api_version (ompd_word_t *version)
> {
> @@ -47,15 +50,24 @@ ompd_get_version_string (const char **string)
> ompd_rc_t
> ompd_initialize (ompd_word_t api_version, const ompd_callbacks_t *callbacks)
> {
> - static int ompd_initialized = 0;
> + if (!callbacks)
> + return ompd_rc_bad_input;
>
> if (ompd_initialized)
> return ompd_rc_error;
>
> + gompd_callbacks = *callbacks;
> +
> (void) api_version;
> - (void) callbacks;
>
> ompd_initialized = 1;
>
> return ompd_rc_ok;
> }
> +
> +ompd_rc_t
> +ompd_finalize (void)
> +{
> + ompd_initialized = 0;
> + return ompd_rc_ok;
> +}
> diff --git a/libgomp/ompd-proc.c b/libgomp/ompd-proc.c
> new file mode 100644
> index 00000000000..6a9c827e327
> --- /dev/null
> +++ b/libgomp/ompd-proc.c
> @@ -0,0 +1,95 @@
> +/* Copyright (C) 2020 Free Software Foundation, Inc.
> + Contributed by Yoosuk Sim <y2s1...@gmail.com>.
> +
> + This file is part of the GNU Offloading and Multi Processing Library
> + (libgomp).
> +
> + Libgomp is free software; you can redistribute it and/or modify it
> + under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3, or (at your option)
> + any later version.
> +
> + Libgomp is distributed in the hope that it will be useful, but WITHOUT ANY
> + WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + more details.
> +
> + Under Section 7 of GPL version 3, you are granted additional
> + permissions described in the GCC Runtime Library Exception, version
> + 3.1, as published by the Free Software Foundation.
> +
> + You should have received a copy of the GNU General Public License and
> + a copy of the GCC Runtime Library Exception along with this program;
> + see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* This file contains function definitions for OMPD's per-process functions
> + defined in the OpenMP 5.0 API Documentation, 5.5.2. */
> +
> +#include <stdlib.h>
> +#include "omp-tools.h"
> +#include "libgompd.h"
> +#include "ompd-types.h"
> +
> +ompd_rc_t
> +ompd_process_initialize (ompd_address_space_context_t *context,
> + ompd_address_space_handle_t **handle)
> +{
> + ompd_rc_t ret = (context) ? ompd_rc_ok : ompd_rc_bad_input;
Formatting, there shouldn't be ()s around context. But, wouldn't it be
better to just return early instead? I.e.
if (context == NULL || handle == NULL)
return ompd_rc_bad_input;
and then you don't need the if later.
> + if (ret == ompd_rc_ok)
> + {
> + ret = gompd_callbacks.alloc_memory (sizeof (ompd_address_space_handle_t),
> + (void **)(handle));
> + }
Formatting. The GNU style says that {s should be indented by 2 and the body
again, so
if (ret == ...)
{
some;
statements;
}
Furthermore, if there is just a single statement, one shouldn't use the {}s.
so
if (ret == ompd_rc_ok)
statement;
Another thing is, there is an aliasing issue in the LLVM libompd, let's not
make the same mistakes. Because the alloc_memory callback will use the
void * type to store the memory address, but handle is
ompd_address_space_handle_t *.
So, you should use
void *ptr;
ret = gompd_callbacks.alloc_memory (sizeof (ompd_address_space_handle_t),
&ptr);
if (ret != omprd_rc_ok)
return ret;
*handle = ptr;
> +
> + if (ret == ompd_rc_ok)
> + {
> + (*handle)->context = context;
> + (*handle)->id = NULL;
> + (*handle)->kind = OMPD_DEVICE_KIND_HOST;
> + }
> +
> + return ret;
> +}
> +
> +ompd_rc_t
> +ompd_device_initialize (ompd_address_space_handle_t *process_handle,
> + ompd_address_space_context_t *device_context,
> + ompd_device_t kind, ompd_size_t sizeof_id, void *id,
> + ompd_address_space_handle_t **device_handle)
Formatting, all the types should be aligned below the first one, while in
your case only the last one is.
> +{
> + ompd_rc_t ret = (process_handle && device_context && id) ? ompd_rc_ok :
> + ompd_rc_bad_input;
Everything I said earlier applies in this function too, but for formatting
I'd also point out that : or ? shouldn't appear at the end of line.
> +ompd_rc_t
> +ompd_rel_address_space_handle (ompd_address_space_handle_t *handle)
> +{
> + ompd_rc_t ret = (handle) ? ompd_rc_ok : ompd_rc_bad_input;
Again, what I said earlier. Furthermore, (*handle).whatever is handle->whatever
> + if (ret == ompd_rc_ok && (*handle).context)
> + ret = gompd_callbacks.free_memory ((*handle).context);
> +#define OMPD_TYPES_VERSION 20180906 /* YYYYMMDD Format */
Where does the 20180906 come from?
> +
> +/* Kinds of device threads */
> +#define OMPD_THREAD_ID_PTHREAD ((ompd_thread_id_t)0)
Space between ) and numbers.
> +#define OMPD_THREAD_ID_LWP ((ompd_thread_id_t)1)
> +#define OMPD_THREAD_ID_WINTHREAD ((ompd_thread_id_t)2)
> +#define OMPD_THREAD_ID_CUDALOGICAL ((ompd_thread_id_t)3)
> +/* The range of non-standard implementation defined values */
> +#define OMPD_THREAD_ID_LO ((ompd_thread_id_t)1000000)
> +#define OMPD_THREAD_ID_HI ((ompd_thread_id_t)1100000)
> +
> +/* Target Cuda device-specific thread identification */
> +typedef struct ompd_dim3_t {
> + ompd_addr_t x;
Only two spaces indentation here.
> + ompd_addr_t y;
> + ompd_addr_t z;
> +} ompd_dim3_t;
> +
> +typedef struct ompd_cudathread_coord_t {
> + ompd_addr_t cudaDevId;
> + ompd_addr_t cudaContext;
> + ompd_addr_t warpSize;
> + ompd_addr_t gridId;
> + ompd_dim3_t gridDim;
Why the two spaces instead of just one here?
> + ompd_dim3_t blockDim;
> + ompd_dim3_t blockIdx;
> + ompd_dim3_t threadIdx;
Jakub
