On Wed, May 18, 2022 at 9:35 AM Jakub Jelinek <ja...@redhat.com> wrote:
> On Mon, May 16, 2022 at 07:35:17PM +0200, Mohamed Atef via Gcc-patches > wrote: > > libgomp/ChangeLog > > > > 2022-05-15 Mohamed Atef <mohamedatef1...@gmail.com> > > > > *config/darwin/plugin-suffix.h (SONAME_SUFFIX): Remove ()s. > > *config/hpux/plugin-suffix.h (SONAME_SUFFIX): Remove ()s. > > *config/posix/plugin-suffix.h (SONAME_SUFFIX): Remove ()s. > > *configure: Regenerate. > > *Makefile.am (toolexeclib_LTLIBRARIES): Add libgompd.la. > > (libgompd_la_LDFLAGS, libgompd_la_DEPENDENCIES, > > libgompd_la_LINK,libgompd_la_SOURCES, libgompd_version_dep, > > libgompd_version_script, libgompd.ver-sun, libgompd.ver, > > libgompd_version_info): New. > > *Makefile.in: Regenerate. > > *aclocal.m4: Regenerate. > > *env.c: Include ompd-support.h. > > (parse_debug): New function. > > (gompd_enabled): New Variable. > > (initialize_env): Call gompd_load. > > (initialize_env): Call parse_debug. > > *team.c: Include ompd-support.h. > > (gomp_team_start): Call ompd_bp_parallel_begin. > > (gomp_team_end): Call ompd_bp_parallel_end. > > (gomp_thread_start): Call ompd_bp_thread_start. > > *libgomp.map: ADD OMP_5.0.3 symbol versions. > > Add rather than ADD > > > *libgompd.map: New. > > *omp-tools.h.in: New. > > *ompd-types.h.in: New. > > *ompd-support.h: New. > > *ompd-support.c: New. > > *ompd-helper.h: New. > > *ompd-helper.c: New. > > *ompd-init.c: New. > > *ompd-icv.c: New. > > *configure.ac (AC_CONFIG_FILES): Add omp-tools.h and ompd-types.h. > > Almost ok, minor comments below. > As I said earlier, as this is just partial implementation of the > OMPD, I think it would be better to commit it to a git branch but > already in the upstream repository, say devel/omp/ompd branch. > > Do you have a FSF Copyright assignment on file or do you want to > submit this under DCO (https://gcc.gnu.org/dco.html)? If the latter, should I remove the header comment? I mean this part " Copyright (C) 2022 Free Software Foundation, Inc. Contributed by Mohamed Atef <mohamedatef1...@gmail.com>. " In fact, I don't know the difference, so which is better for the community? > If the latter, > we need Signed-off-by line in your final patch mail and also in the > commit message. > The commit message should include a one line summary, then > empty line, then explanation what the patch is, followed by properly > formatted ChangeLog entry (the above is badly mangled by your mailer), > there should be 2 spaces around the name etc. and after the ChangeLog > entry the Signed-of-by line if you use DCO. > More details in https://gcc.gnu.org/gitwrite.html > In Authenticated access there is a link to a form to request write > access to the repository, please file it and write me as the sponsor. > Then follow the rest of gitwrite to e.g. do > contrib/gcc-git-customization.sh > and then you can git gcc-verify to verify whether the ChangeLog entry in > your commit log is ok before you push it. > > > --- /dev/null > > +++ b/libgomp/libgompd.map > > @@ -0,0 +1,23 @@ > > +OMPD_5.1 { > > Shouldn't this be OMPD_5.0 ? > The functions were already in OpenMP 5.0, weren't they? > > > + global: > > + ompd_initialize; > > + ompd_get_api_version; > > + ompd_get_version_string; > > + ompd_process_initialize; > > + ompd_device_initialize; > > + ompd_rel_address_space_handle; > > + ompd_finalize; > > + ompd_enumerate_icvs; > > + ompd_get_icv_from_scope; > > + ompd_get_icv_string_from_scope; > > + ompd_get_thread_handle; > > + ompd_get_thread_in_parallel; > > + ompd_rel_thread_handle; > > + ompd_thread_handle_compare; > > + ompd_get_thread_id; > > + ompd_get_device_from_thread; > > + ompd_bp_thread_begin; > > + ompd_bp_thread_end; > > Why is ompd_bp_thread_{begin,end} here? I thought they were > in libgomp.so.*, not in libgompd.so.* > > > + local: > > + *; > > +}; > > > + /* NO cuda for now. */ > > We support other kinds of offloading, so better just say > /* No offloading support for now. */ > or so. > > > + if (device == OMPD_DEVICE_KIND_HOST) > > + { > > + switch (icv_id) > > + { > > + case gompd_icv_cancellation_var: > > + return > > + gompd_get_cancellation ((ompd_address_space_handle_t *) > handle, > > + icv_value); > > + case gompd_icv_max_task_priority_var: > > + return gompd_get_max_task_priority > ((ompd_address_space_handle_t *) > > + handle, icv_value); > > + case gompd_icv_stacksize_var: > > + return gompd_get_stacksize ((ompd_address_space_handle_t *) > handle, > > + icv_value); > > You use that overly long (ompd_address_space_handle_t *) handle in all the > spots and it causes various formatting issues. Wouldn't it be better > to add some temporary variable above the switch > ompd_address_space_handle_t *whatever > = (ompd_address_space_handle_t *) handle; > (for some good choice of name) and then just use it instead of > (ompd_address_space_handle_t *) handle which would allow always writing > return gompd_whatever (whatever, icv_value); > and not the uglier return with call on the next line, or even worse ( on > a different line from the function name? > > > +ompd_rc_t > > +ompd_finalize () > > Please use (void) instead of () > > > +void > > +gompd_load () > > Likewise. > > > +void __attribute__ ((noipa)) > > +ompd_dll_locations_valid () > > Likewise (and several times more). > > Fixed. > Jakub > >
