From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id 600503858D20 for ; Tue, 15 Mar 2022 00:21:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 600503858D20 Received: by mail-wr1-x430.google.com with SMTP id i8so26566400wrr.8 for ; Mon, 14 Mar 2022 17:21:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=GzdQ0BYxBTKptXMRtm/fsEtaAkfwR2cmDaKSjH8VNzI=; b=71uoLSLeMnmdBGBzKLIfnyss1LPJ3P3leqTYUvtjNlr5DQaicwep89kZLR5mYWYeY8 CJRGTyErbFqy7VUETqJtzHBsVYcN7ScwFpwp2QkN3WMipNnMTOBmhCo3r8p3+lsvkW18 dDCj269XU7dS9ZLjsZsaUD7knc749Wl5kD/UpRseqneVyCVupGuyHtL+WxGBH+FwLTai 8vD9fH9Q29bl3ELO/flT+reQVsagruhVfc29Blwyi91mQx0Z6jcqJoXg7zAlegV7DX+n Z9nXnzpUP9HuIYLCV4Zy5v2XJuxgZ2Upaef69Jd805nACRUxqKN5NZyWtxQ0SLOShNQP 4r2Q== X-Gm-Message-State: AOAM533GoWTWNYDi+qbGVADJzW2OcBjZyKTDmULhro2LvG0lFpcrfuLC 7rnEaj4B/h2yLaQX0aHvFzYXU54rBAj4dmpTl64= X-Google-Smtp-Source: ABdhPJwbYsYkliuL1FFjvxalkNdburMuzjV9SuZQPs3AcLGU6kJSS2pu/x3uvu2kbMyuzKUrZY45g8TAqUqfEzRO1PQ= X-Received: by 2002:a5d:490f:0:b0:1f0:6791:a215 with SMTP id x15-20020a5d490f000000b001f06791a215mr18554787wrq.422.1647303689929; Mon, 14 Mar 2022 17:21:29 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Mohamed Atef Date: Tue, 15 Mar 2022 02:21:18 +0200 Message-ID: Subject: Re: [PATCH] libgomp : OMPD implementation To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org, Martin Jambor , tobias@codesourcery.com X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Mar 2022 00:21:38 -0000 Hi Jakub, i will fix all the issues and i will use the script on the website. On Mon, Mar 14, 2022 at 6:23 PM Jakub Jelinek wrote: > Hi! > > Sorry for the delay, GCC is currently in stage 4, which means a lot of us > concentrate on fixing GCC 12 so that it can be released soon and projects > that are clearly GCC 13 material are much lower priority. > Never mind, thank you anyway. > > On Wed, Feb 16, 2022 at 11:04:13PM +0200, Mohamed Atef via Gcc-patches > wrote: > > --- a/libgomp/ChangeLog > > +++ b/libgomp/ChangeLog > > @@ -1,3 +1,30 @@ > > ChangeLog entries should be posted in the patches and eventually should b= e > in the commit message, but the patch shouldn't change ChangeLog files > directly, we have nightly scripts that handle that. > > > +2022-02-16 Mohamed Atef > > + > > + * Makefile.am (toolexeclib_LTLIBRARIES): Add libgompd.la. > > For ChangeLog entries, https://gcc.gnu.org/contribute.html specifies > how it should be formatted. > E.g. there should be a tab instead of 8 spaces at the start of the lines > (except for the date/name/email line), the above has spaces. > > > + (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): > Defined. > > No trailing whitespace. I think it would be better to use : New. > > > + * Makefile.in: Regenerate. > > + * aclocal.m4: Regenerate. > > + * config/darwin/plugin-suffix.h: Removed (). > > + * config/hpux/plugin-suffix.h: Removed (). > > + * config/posix/plugin-suffix.h: Removed (). > > These don't say what has changed, and the tense is wrong. So should be > like: > * config/posix/plugin-suffix.h (SONAME_SUFFIX): Remove ()s. > or so. > > > + * configure: Regenerate. > > + * env.c: (#include "ompd-support.h") : Added. > > The above correctly mentions just the filename and not a particular thing > in it for the added include, but the rest is incorreclty formatted. > Should be: > * env.c: Include ompd-support.h > or so. > > > + (initialize_env) : Call ompd_load(). > > There should be no space in between ) and :. Just say > (initialize_env): Call ompd_load. > No need for the ()s in there. > > > + * parallel.c:(#include "ompd-support.h"): Added. > > See above. > > > + (GOMP_parallel) : Call ompd_bp_parallel_begin and > ompd_bp_parallel_end. > > + * libgomp.map: Add OMP_5.0.3 symobl versions. > > + * libgompd.map: New file. > > + * omp-tools.h.in : New file. > > + * omp-types.h.in : New file. > > + * ompd-support.h : New file. > > + * ompd-support.c : New file. > > + * ompd-helper.h : New file. > > See above for the superfluous spaces before :. > > > + * ompd-helper.c: New file. > > + * ompd-init.c: New file. > > + * testsuite/Makfile.in: Regenerate. > > Typo, Makefile.in instead of Makfile.in (the checking script would preven= t > commit because of this). > > > --- a/libgomp/Makefile.am > > +++ b/libgomp/Makefile.am > ... > > +libgompd_version_info =3D -version-info $(libtool_VERSION) > > libgomp_la_LDFLAGS =3D $(libgomp_version_info) $(libgomp_version_scrip= t) \ > > $(lt_host_flags) > > +libgompd_la_LDFLAGS =3D $(libgompd_version_info) > $(libgompd_version_script) \ > > + $(lt_host_flags) > > libgomp_la_DEPENDENCIES =3D $(libgomp_version_dep) > > +libgompd_la_DEPENDENCIES =3D $(libgompd_version_dep) > > libgomp_la_LINK =3D $(LINK) $(libgomp_la_LDFLAGS) > > - > > +libgompd_la_LINK =3D $(LINK) $(libgompd_la_LDFLAGS) > > Please preserve the empty line above libgomp_la_SOURCES. > > > --- a/libgomp/Makefile.in > > +++ b/libgomp/Makefile.in > > @@ -1,7 +1,7 @@ > > -# Makefile.in generated by automake 1.15.1 from Makefile.am. > > +# Makefile.in generated by automake 1.16.1 from Makefile.am. > > Please make sure you regenerate files with the exact autoconf/automake > versions as used previously, we don't want the generated files > jump backwards/forwards whenever somebody regenerates them. > You can e.g. build automake 1.15.1 and autoconf 2.69 if you don't have > those, make install DESTDIR=3D/your/home/automake-1.15.1 etc. > and then just regenerate with those in $PATH. > done. > > --- a/libgomp/env.c > > +++ b/libgomp/env.c > > @@ -1483,6 +1484,8 @@ initialize_env (void) > > =3D thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var; > > } > > parse_int_secure ("GOMP_DEBUG", &gomp_debug_var, true); > > + if(gomp_debug_var) > > + ompd_load(); > > Formatting. Again, https://gcc.gnu.org/contribute.html contains > the rules or just watch how does surrounding code look like. > The errors are: > 1) space in between if and ( is missing > 2) ompd_load should be indented 2 further columns from if (and > any consecutive 8 spaces at the start of lines replaced with > tab in *.c/*.C files) > 3) space between ompd_load and ( is missing > > do macros also should have a space between the name and the '(' > > #ifndef HAVE_SYNC_BUILTINS > > gomp_mutex_init (&gomp_managed_threads_lock); > > #endif > > diff --git a/libgomp/libgomp.map b/libgomp/libgomp.map > > index 2ac58094169..5c57b1a2d08 100644 > > --- a/libgomp/libgomp.map > > +++ b/libgomp/libgomp.map > > @@ -226,6 +226,16 @@ OMP_5.1 { > > omp_get_teams_thread_limit_; > > } OMP_5.0.2; > > > > +OMP_5.0.3 { > > + global: > > + ompd_dll_locations; > > + ompd_dll_locations_valid; > > + ompd_bp_parallel_begin; > > + ompd_bp_parallel_end; > > + local: > > + *; > > +}OMP_5.0.2; > > Again, please watch how the surrounding code looks like. > The first 2 added lines are correct, the next 4 have 2 spaces > followed by tab when they should start with just tab, local: *; lines > shouldn't be there, that is just in the very first symver and nowhere els= e. > And there should be a space between } and OMP_5.0.2; > > > --- /dev/null > > +++ b/libgomp/libgompd.map > > @@ -0,0 +1,12 @@ > > +OMPD_5.1 { > > + global: > > + ompd_initialize; > > + ompd_get_api_version; > > + ompd_get_version_string; > > + ompd_process_initialize; > > + ompd_device_initialize; > > + ompd_rel_address_space_handle; > > + ompd_finalize; > > + local: > > + *; > > +}; > > The *; local has incorrect indentation, the rest is ok. > > > --- /dev/null > > +++ b/libgomp/omp-tools.h.in > > > +void ompd_dll_locations_valid(void); > > Space before (. > > + > > +typedef __UINT64_TYPE__ ompd_size_t; > > +typedef __UINT64_TYPE__ ompd_wait_id_t; > > + > > +typedef __UINT64_TYPE__ ompd_addr_t; > > +typedef __INT64_TYPE__ ompd_word_t; > > +typedef __UINT64_TYPE__ ompd_seg_t; > > + > > +typedef struct ompd_address_t > > +{ > > + ompd_seg_t segment; > > + ompd_addr_t address; > > These should be indented by 2 spaces instead of a tab. > > +} ompd_address_t; > > + > > +typedef struct ompd_frame_info_t > > +{ > > + ompd_address_t frame_address; > > + ompd_word_t frame_flag; > > Ditto. > > > +typedef struct _ompd_task_handle ompd_task_handle_t; > > + > > +typedef enum ompd_scope_t > > +{ > > + ompd_scope_global =3D 1, > > + ompd_scope_address_space =3D 2, > > + ompd_scope_thread =3D 3, > > + ompd_scope_parallel =3D 4, > > + ompd_scope_implicit_task =3D 5, > > + ompd_scope_task =3D 6 > > Likewise. And just space =3D space before the values. > Several times in the file. > > > + > > +typedef ompd_rc_t > (*ompd_callback_get_thread_context_for_thread_id_fn_t) > > No trailing whitespace. > > > + (ompd_address_space_context_t *, ompd_thread_id_t, > > + ompd_size_t, const void *, ompd_thread_context_t **); > > I'd indent the ( just by 2 spaces, and ompd_size_t should be indented > below ompd_address_space_context_t, i.e. not below (. > Again many times. > > + > > +#endif /* _OMP_TOOLS_H */ > > \ No newline at end of file > > Please make sure newly added files are newline terminated. > > > diff --git a/libgomp/ompd-helper.c b/libgomp/ompd-helper.c > > new file mode 100644 > > index 00000000000..033990073a5 > > --- /dev/null > > +++ b/libgomp/ompd-helper.c > > @@ -0,0 +1,47 @@ > > +/* Copyright (C) 2021 Free Software Foundation, Inc. > > It is 2022 now. > > > + . */ > > + > > + > > + > > + > > + > > Please avoid the excessive vertical whitespace, 2 lines is ok, > more than that is not. > > > +#include "ompd-helper.h" > > + > > +ompd_device_type_sizes_t target_sizes; > > + > > +ompd_rc_t > > +get_sizes(ompd_address_space_context_t *context) > > Space before ( etc., incorrect indentation, what I said above. > > > +{ > > + if(context =3D=3D NULL) > > + return ompd_rc_bad_input; > > > + static int inited =3D 0; > > Just use bool and false (and include somewhere). > Also, can that be called in parallel by multiple threads? > If so, such inited checking is racy. > > from OpenMP specs. "The OMPD library does not need to be reentrant. The tool must ensure that only one thread enters the OMPD library at a time. The OMPD library must not install signal handlers or otherwise interfere with the tool=E2=80=99s signal configuration." > > +/*This file contains the implementation of functions defined in > > + section 5.5.1, 5.5.2. */ > > Space after /* missing, and section should be below This. > > > +ompd_rc_t > > +ompd_initialize(ompd_word_t api_version, > > + const ompd_callbacks_t *callbacks_table) > > Space before ( and const ompd_ should be below ompd_word_t. > > > + static const char tmp_string[] =3D > > =3D should be on the next line already. > > > + "GNU OpenMP runtime implementing OMPD version " > > + stringize(VERSION) " Debugging library."; > > stringize (VERSION) should be below that first ", i.e. > static const char tmp_string[] > =3D "GNU OpenMP runtime implementing OMPD version " > stringize (VERSION) " Debugging library"; > The . at the end makes no sense, it isn't a sentence. > > > + //naive way to read from memory > > We use just C comments in libgomp, missing capital at the start > and . at the end, so > /* Native way to read from memory. */ > > > + ret =3D callbacks->symbol_addr_lookup(context, NULL, "ompd_state"= , > > + &symbol_addr, NULL); > > Arguments on next line should be below arguments on the first line, so > one column after (. > > + > > + ret =3D callbacks->read_memory(context, NULL, &symbol_addr, > > + target_sizes.sizeof_long_long, &ompd_state); > > + > > + ret =3D callbacks->device_to_host(context, &ompd_state, > > + target_sizes.sizeof_long_long, 1, &ompd_state); > > + > > + ret =3D callbacks->alloc_memory(sizeof(ompd_address_space_handle_= t), > > + (void **)(handle)); > > Space between )(. Also, shouldn't ret !=3D ompd_rc_ok be checked after > every call? Otherwise you only checkit from the last one. > > + > > + > > + if(ret !=3D ompd_rc_ok) > > + return ret; > > + > > + if(handle =3D=3D NULL) > > + return ompd_rc_error; > > + > > + (*handle)->context =3D context; > > + (*handle)->kind =3D OMPD_DEVICE_KIND_HOST; > > + return ret; > > +} > > + > > + > > + > > +/*OMPD will not support GPUs for now. */ > > + > > +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) > > + > > +{ > > + if(device_context =3D=3D NULL) > > + return ompd_rc_bad_input; > > + > > + return ompd_rc_unsupported; > > +} > > + > > + > > + > > +ompd_rc_t > > +ompd_rel_address_space_handle(ompd_address_space_handle_t *handle) > > +{ > > + if(handle =3D=3D NULL) > > + return ompd_rc_stale_handle; > > + > > + ompd_rc_t ret =3D callbacks->free_memory((void *)handle); > > + return ret; > > +} > > diff --git a/libgomp/ompd-support.c b/libgomp/ompd-support.c > > new file mode 100644 > > index 00000000000..8458e1f5d5f > > --- /dev/null > > +++ b/libgomp/ompd-support.c > > @@ -0,0 +1,108 @@ > > +/* Copyright (C) 2021 Free Software Foundation, Inc. > > + Contributed by Mohamed Atef . > > + This file is part of the GNU Offloading and Multi Processing Librar= y > > + (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 a= nd > > + a copy of the GCC Runtime Library Exception along with this program= ; > > + see the files COPYING3 and COPYING.RUNTIME respectively. If not, s= ee > > + . */ > > + > > +#include "ompd-support.h" > > + > > + > > + > > +const char **ompd_dll_locations =3D NULL; > > +__UINT64_TYPE__ ompd_state; > > + > > +void > > +ompd_load() > > +{ > > + static int ompd_initialized =3D 0; > > + if(ompd_initialized) > > + return; > > + fprintf(stderr, "OMP OMPD active\n"); > > I think you don't want such printouts, perhaps use gomp_debug so that > it is only printed if user asks for it. > Also, ompd_load is not a symbol specified in the standard, so we should > call it differently, the omp_* and ompd_* prefixes should be reserved > for symbols from the standard. > And, now I see you call ompd_load guarded with gomp_debug_var, shouldn't > that be guarded with a var set from parsing "OMP_DEBUG" env var rather > than the implementation specific "GOMP_DEBUG" which means something > completely else? > I didn't find OMP_DEBUG in the list of environment variables. but fixed. > Why do you need that ompd_initialized stuff? env.c calls it just once. > > > + static const char *tmp_ompd_dll_locations[2] > > + =3D {"libgompd" SONAME_SUFFIX(1) , NULL}; > > No space before , and =3D should be 2 columns to the right from static > which should be 2 columns to the right from { (this is everywhere in the > patch). > > > + ompd_state |=3D OMPD_ENABLED; > > + ompd_initialized =3D 1; > > + ompd_dll_locations =3D (const char **)malloc(2 * sizeof(const cha= r > *)); > > If you'd need to actually allocate something, you'd want to use > gomp_malloc and similar APIs that will fail miserably if memory can't be > allocated. But another option is just to have a static 2 elements array > that holds it and just set ompd_dll_locations to the address of that > array's > first element. After all, you clearly have one like that, > just don't call it tmp_ompd_dll_locations but say ompd_dll_locations_arra= y > or something similar. If you for whatever reason need to allocate it > from heap, on the other side don't make that array static, we really want > as few dynamic relocations in the binary and as small .data and .rodata > as possible. > > > +void __attribute__ ((noinline)) > > +ompd_dll_locations_valid() > > +{ > > I'd strongly prefer these not to be separate functions at least > on the common platforms. At least when they are called just once > instead of multiple times. > So have in the headers some macros that depending on e.g. whether > it is __ELF__ define those as: > #ifdef __ELF__ > #define ompd_dll_locations_valid() \ > __asm__ __volatile__ (".globl ompd_dll_locations_valid\n\t" > "ompd_dll_locations_valid:"); > #else > extern void ompd_dll_locations_valid (void); > #endif > So should I remove them from the omp-tools.h file? > and use these out of line functions only if the macros aren't defined. > > @@ -173,10 +174,14 @@ GOMP_parallel (void (*fn) (void *), void *data, > unsigned num_threads, > > unsigned int flags) > > { > > num_threads =3D gomp_resolve_num_threads (num_threads, 0); > > + if(ompd_state) > > + ompd_bp_parallel_begin(); > > This shouldn't be conditional on anything (especially if it is just asm > volatile with a label rather than a function call, because it doesn't cos= t > anything in that case except for the (required) .dynsym entry). > > But, I think this spot is also not the best one, because it isn't the onl= y > one invoked for #pragma omp parallel. GOMP_parallel_loop_static_start, > GOMP_parallel_loop_nonmonotonic_guided etc., there are many. > Better to stick it into gomp_team_start and gomp_team_end. > > done, what about ompd_thread_bp_begin, and ompd_thread_bp_end : where should i put them? > Please adjust your other patch with all the issues raised above too. > > Jakub > >