On Fri, Jul 03, 2020 at 10:43:55PM -0400, y2s1982 via Gcc-patches wrote:
> This patch adds OMPD functions defined in 5.5.2 of the OpenMP 5.0 API
> documentation. It adds per-process and per-device functions, defines
> related handle data types, and adds a helper function for storing device
> id.
>
> 2020-07-03 Tony Sim <y2s1...@gmail.com>
>
> libgomp/ChangeLog:
>
> * Makefile.am (libgompd_la_OBJECTS): Add ompd-proc.c and
> ompd-helper.c
Formatting. If ompd-helper.c doesn't fit, then it would go below
the * on next line (i.e. a single tab indented).
And period at the end. But I think it still fits.
* Makefile.am (libgompd_la_OBJECTS): Add ompd-proc.c and ompd-helper.c.
> +ompd_rc_t
> +gompd_get_gompd_device_id (void *input_id, ompd_size_t id_size,
> + _gompd_device_id *output_id)
Identifiers starting with underscore are reserved for implementation, so
better avoid them if they are private to the implementation.
> + switch (id_size)
> + {
> + case 1:
> + *output_id = (_gompd_device_id) *((__UINT8_TYPE__ *) input_id);
> + break;
> + case 2:
> + *output_id = (_gompd_device_id) *((__UINT16_TYPE__ *) input_id);
I have no idea what this function is doing, but e.g. from aliasing point of
view trying to access something as short/int/long long is dangerous, and
there might be alignment implications too.
> --- /dev/null
> +++ b/libgomp/ompd-helper.h
> @@ -0,0 +1,37 @@
> +/* 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 header declares helper functions of OMPD library. */
IMHO you don't need hundreds of headers, putting most OMPD related private
stuff in libgompd.h should be enough.
> +ompd_rc_t gompd_get_gompd_device_id (void *, ompd_size_t, _gompd_device_id *)
> + __GOMPD_NOTHROW;
The __GOMPD_NOTHROW; should be indented by 2 spaces from line start if it
doesn't fit after the ).
> +ompd_rc_t
> +ompd_process_initialize (ompd_address_space_context_t *context,
> + ompd_address_space_handle_t **handle)
> +{
> + ompd_rc_t ret = handle ? ompd_rc_ok : ompd_rc_stale_handle;
> + if (ret != ompd_rc_ok)
> + return ret;
> +
> + ret = context ? ompd_rc_ok : ompd_rc_bad_input;
Why so complicated? Just do:
ompd_rc_t ret;
if (handle == NULL)
return ompd_rc_stale_handle;
if (context == NULL)
return ompd_rc_bad_input;
> +ompd_rc_t
> +ompd_rel_address_space_handle (ompd_address_space_handle_t *handle)
> +{
> + ompd_rc_t ret = handle && handle->context
> + ? ompd_rc_ok : ompd_rc_stale_handle;
> + if (ret != ompd_rc_ok)
> + return ret;
> +
> + ret = handle->ref_count == 0 ? ompd_rc_ok : ompd_rc_unavailable;
> + if (ret != ompd_rc_ok)
> + return ret;
Similarly.
> --- /dev/null
> +++ b/libgomp/ompd-types.h
ompd-types.h is an installed header I think, so Makefile.am should install
it.
Jakub
