On Tue, Oct 10, 2023 at 12:59:52PM +0800, Jun Zhang wrote:
> include/ChangeLog:
>
> * omphook.h: define RUNOMPHOOK macro.
ChangeLog formatting. The description should start with
capital letter. If you add a new file, you should just
mention : New file. or something similar.
But much more importantly, we don't do hooks like that in libgomp,
it should be better a static inline function where the name shows
what it is doing (e.g. do_adjust_default_spincount), best in some
already existing header (e.g. wait.h), and best at the end of the
if (!parse_spincount ("GOMP_SPINCOUNT", &gomp_spin_count_var))
block in env.c.
>
> libgomp/ChangeLog:
>
> * env.c (initialize_env): add RUNOMPHOOK macro.
> * config/linux/x86/omphook.h: define RUNOMPHOOK macro.
> ---
> include/omphook.h | 1 +
> libgomp/config/linux/x86/omphook.h | 19 +++++++++++++++++++
> libgomp/env.c | 3 +++
> 3 files changed, 23 insertions(+)
> create mode 100644 include/omphook.h
> create mode 100644 libgomp/config/linux/x86/omphook.h
>
> diff --git a/include/omphook.h b/include/omphook.h
> new file mode 100644
> index 00000000000..2ebe3ad57e6
> --- /dev/null
> +++ b/include/omphook.h
> @@ -0,0 +1 @@
> +#define RUNOMPHOOK()
> diff --git a/libgomp/config/linux/x86/omphook.h
> b/libgomp/config/linux/x86/omphook.h
> new file mode 100644
> index 00000000000..aefb311cc07
> --- /dev/null
> +++ b/libgomp/config/linux/x86/omphook.h
> @@ -0,0 +1,19 @@
> +#ifdef __x86_64__
> +#include "cpuid.h"
> +
> +/* only for x86 hybrid platform */
Comments should again start with capital letter and end with dot and 2
spaces before */
> +#define RUNOMPHOOK() \
> + do \
> + { \
> + unsigned int eax, ebx, ecx, edx; \
> + if ((getenv ("GOMP_SPINCOUNT") == NULL) && (wait_policy < 0) \
Spurious ()s around the subcondition, the first shouldn't be tested when
placed right, if condition doesn't fit on one line, each subcondition
should be on separate line.
> + && __get_cpuid_count (7, 0, &eax, &ebx, &ecx, &edx) \
If we don't have a macro for the CPUID.07H.0H:EDX[15] bit, there should
be a comment which says what that bit is.
> + && ((edx >> 15) & 1)) \
> + gomp_spin_count_var = 1LL; \
> + if (gomp_throttled_spin_count_var > gomp_spin_count_var) \
> + gomp_throttled_spin_count_var = gomp_spin_count_var; \
The above 2 lines won't be needed if placed right.
Jakub
