Hello,
Thank you for the detailed reply. I have some questions.
On Thu, Jul 2, 2020 at 8:47 AM Jakub Jelinek <ja...@redhat.com> wrote:
> 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).
>
I thought I understood this part, but I just need a little more nudge. I
get that standard left it
to the discretion to the implementation, and I assumed we would be the ones
to define
them. Is it okay to define the handle? Also, is the context only stored in
the OMPD and
is only used when passed back to the debugger so that debugger understands
the context
to the information OMPD gave back to it? Does that mean any of the
functions I use should
not try to get information from it? Does this also mean I shouldn't free
up the context pointers
when releasing the handle as the information is handled by the debugger?
>
> > --- 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).
>
I didn't get any reply and wasn't sure if the patch was approved.
I also needed the gompd_callbacks to define the functions.
I then thought they were small enough and similar enough changes to
combine into a single patch. Should I keep them separated?
> > #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.
>
Okay. I thought of having just a single return statement per function, but
I will change them.
I do have a question: as handle is considered the output of the function,
should
I still assume it is not NULL when passed in?
> + 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?
>
Much of the ompd-types.h comes from
https://github.com/OpenMP/sources/blob/master/include/ompd-types.h.
OMPD_TYPES_VERSION was a part of it. Should I change the value as I
added more to this, or should I separate the custom definition to a
separate file
to keep the version consistent with OpenMP?
Also, most of the formatting issues in the file were the result of the
direct
copy/paste from their code. While I did change the 8 spaces to a tab
character,
it seems there are more to fix. I will fix them.
> +
> > +/* 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
>
>
