From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id B4EF5385734A for ; Fri, 29 Apr 2022 13:46:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B4EF5385734A Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-134-jxXbCcZsMj2Zx5ZF64q2Ew-1; Fri, 29 Apr 2022 09:46:14 -0400 X-MC-Unique: jxXbCcZsMj2Zx5ZF64q2Ew-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 967DF29AB3E0; Fri, 29 Apr 2022 13:46:14 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 56C0D455921; Fri, 29 Apr 2022 13:46:14 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 23TDkBFw3153700 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 29 Apr 2022 15:46:12 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 23TDkBAj3153699; Fri, 29 Apr 2022 15:46:11 +0200 Date: Fri, 29 Apr 2022 15:46:10 +0200 From: Jakub Jelinek To: Mohamed Atef Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] libgompd: add OMPD support, libgompd initialization and global ICVs functions Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.85 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: Fri, 29 Apr 2022 13:46:23 -0000 On Sun, Mar 20, 2022 at 11:33:10AM +0200, Mohamed Atef via Gcc-patches wrote: > hello, > I know it's too much. > we fixed the functions' names that are not part of the standard form ompd_ > * prefix to gompd_ Sorry for the delay, have been busy with GCC 12. Now that it branched, I can look at this again. Will try to do timely reviews now that GCC 13 is in stage1. > > 2022-03-15 Mohamed Atef Note, you'll need to update the ChangeLog entry for the changes in the patch. > --- a/libgomp/Makefile.am > +++ b/libgomp/Makefile.am > @@ -32,13 +32,21 @@ libgomp.ver: $(top_srcdir)/libgomp.map > $(EGREP) -v '#(#| |$$)' $< | \ > $(PREPROCESS) -P -include config.h - > $@ || (rm -f $@ ; exit 1) > > +libgompd.ver: $(top_srcdir)/libgompd.map > + $(EGREP) -v '#(#| |$$)' $< | \ > + $(PREPROCESS) -P -include config.h - > $@ || (rm -f $@ ; exit 1) Why the different indentation from the entry you've copied it from? Better stay consistent. > +libgompd.ver-sun : libgompd.ver \ > + $(top_srcdir)/../contrib/make_sunver.pl \ > + $(libgompd_la_OBJECTS) $(libgompd_la_LIBADD) > + perl $(top_srcdir)/../contrib/make_sunver.pl \ > + libgompd.ver \ > + $(libgompd_la_OBJECTS:%.lo=.libs/%.o) \ > + `echo $(libgompd_la_LIBADD) | \ > + sed 's,/\([^/.]*\)\.la,/.libs/\1.a,g'` \ > + > $@ || (rm -f $@ ; exit 1) Likewise. > --- a/libgomp/env.c > +++ b/libgomp/env.c > @@ -33,6 +33,7 @@ > #ifndef LIBGOMP_OFFLOADED_ONLY > #include "libgomp_f.h" > #include "oacc-int.h" > +#include "ompd-support.h" > #include > #include > #include > @@ -1483,6 +1484,7 @@ initialize_env (void) > = thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var; > } > parse_int_secure ("GOMP_DEBUG", &gomp_debug_var, true); > + gompd_load (); See below. > +#ifndef _OMP_TOOLS_H > +#define _OMP_TOOLS_H > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +extern const char **ompd_dll_locations; > + > +#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 /* __ELF__ */ In the public omp-tools.h, ompd_dll_locations_valid should be declared just as extern void ompd_dll_locations_valid (void); (well, extern void ompd_dll_locations_valid (void) __GOMPD_NOTHROW; and there should be #ifdef __cplusplus extern "C" { # define __GOMPD_NOTHROW throw () #else # define __GOMPD_NOTHROW __attribute__((__nothrow__)) #endif earlier. The fact that we want the former macro if possible is an implementation detail we shouldn't expose to users in a public header. So that #define belongs to some non-public header, ideally using some helper macro because you use it many times later. > +ompd_rc_t ompd_initialize (ompd_word_t, const ompd_callbacks_t *); Similarly add __GOMPD_NOTHROW to this and other declarations. > + if (current + 1 >= gompd_last_icv_var || next_id == NULL > + || next_icv_name == NULL || next_scope == NULL || more == NULL) The general formatting rule is if all the whole if/while condition fits on one line, use just one line, otherwise put each ||/&& part on a separate line. So if (whatever || whatever2 || whatever3) if short or if (whatever || whatever2 || whatever3 || whatever4 || whatever5) if too long. > +void > +gompd_load () > +{ > + const char *ompd_env_var = getenv ("OMP_DEBUG"); > + if (ompd_env_var == NULL || strcmp (ompd_env_var, "enabled")) > + return; If you look at other similar env var handling, you can notice that we accept leading and/or trailing whitespace and use case insensitive comparisons for strings. See e.g. env.c (parse_target_offload). IMHO the OMP_DEBUG env var parsing should be done in env.c too next to other env vars in a similar way how other vars are handled and gompd_load only called if OMPD is enabled. > +#ifndef __ELF__ > +/* Dummy functions. they shoud not be optimized. */ > + > +void __attribute__ ((noinline)) noinline isn't strong enough, it should use noipa attribute here and in the similar spots. > + > +void gompd_load (); We want internal functions like this (note, (void) rather than (), this is C) to be marked hidden if possible, libgomp.h uses #ifdef HAVE_ATTRIBUTE_VISIBILITY # pragma GCC visibility push(hidden) #endif ... #ifdef HAVE_ATTRIBUTE_VISIBILITY # pragma GCC visibility pop #endif Jakub