Hi!
On Sat, 10 Nov 2018 09:11:18 -0800, Julian Brown <jul...@codesourcery.com>
wrote:
> This patch (by Cesar, with some minor additional changes)
Cesar's changes we're handling separately (already approved; will commit
soon), so it remains here:
> replaces usage
> of several magic constants in target.c with named macros
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -902,6 +902,11 @@ struct target_mem_desc {
> artificial pointer to "omp declare target link" object. */
> #define REFCOUNT_LINK (~(uintptr_t) 1)
>
> +/* Special offset values. */
> +#define OFFSET_INLINED (~(uintptr_t) 0)
> +#define OFFSET_POINTER (~(uintptr_t) 1)
> +#define OFFSET_STRUCT (~(uintptr_t) 2)
> +
> struct splay_tree_key_s {
> /* Address of the host object. */
> uintptr_t host_start;
I'd move these close to the struct they apply to.
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -45,6 +45,8 @@
> #include "plugin-suffix.h"
> #endif
>
> +#define FIELD_TGT_EMPTY (~(size_t) 0)
> +
> static void gomp_target_init (void);
>
> /* The whole initialization code for offloading plugins is only run one. */
As it's only used there, I'd actually move that one into "gomp_map_vars",
as a "const size_t field_tgt_empty". And, you'd missed to use it in the
initialization of "field_tgt_clear". ;-)
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -876,6 +892,8 @@ gomp_map_vars_async (struct gomp_device_descr *devicep,
> else
> k->host_end = k->host_start + sizeof (void *);
> splay_tree_key n = splay_tree_lookup (mem_map, k);
> + /* Need to account for the case where a struct field hasn't been
> + mapped onto the accelerator yet. */
> if (n && n->refcount != REFCOUNT_LINK)
> gomp_map_vars_existing (devicep, aq, n, k, &tgt->list[i],
> kind & typemask, cbufp);
We usually talk about "device", not "accelerator".
All that I'm changing with the incremental patch attached.
I'm also again attaching the complete patch that we'd like to commit to
trunk; Jakub, OK? If approving this patch, please respond with
"Reviewed-by: NAME <EMAIL>" so that your effort will be recorded in the
commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>.
Grüße
Thomas
>From 8f36a7d620b3e1d0130b352dc02d58c066c7ba92 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Fri, 21 Dec 2018 11:28:49 +0100
Subject: [PATCH] [WIP] libgomp/target.c magic constants self-documentation
---
libgomp/libgomp.h | 10 +++++-----
libgomp/target.c | 11 +++++------
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 19e5fbb24e26..eef380d7b0fc 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -873,6 +873,11 @@ struct target_var_desc {
uintptr_t length;
};
+/* Special values for struct target_var_desc's offset. */
+#define OFFSET_INLINED (~(uintptr_t) 0)
+#define OFFSET_POINTER (~(uintptr_t) 1)
+#define OFFSET_STRUCT (~(uintptr_t) 2)
+
struct target_mem_desc {
/* Reference count. */
uintptr_t refcount;
@@ -903,11 +908,6 @@ struct target_mem_desc {
artificial pointer to "omp declare target link" object. */
#define REFCOUNT_LINK (~(uintptr_t) 1)
-/* Special offset values. */
-#define OFFSET_INLINED (~(uintptr_t) 0)
-#define OFFSET_POINTER (~(uintptr_t) 1)
-#define OFFSET_STRUCT (~(uintptr_t) 2)
-
struct splay_tree_key_s {
/* Address of the host object. */
uintptr_t host_start;
diff --git a/libgomp/target.c b/libgomp/target.c
index d7acdd9b784b..201da567d73a 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -45,8 +45,6 @@
#include "plugin-suffix.h"
#endif
-#define FIELD_TGT_EMPTY (~(size_t) 0)
-
static void gomp_target_init (void);
/* The whole initialization code for offloading plugins is only run one. */
@@ -748,7 +746,8 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
if (not_found_cnt)
tgt->array = gomp_malloc (not_found_cnt * sizeof (*tgt->array));
splay_tree_node array = tgt->array;
- size_t j, field_tgt_offset = 0, field_tgt_clear = ~(size_t) 0;
+ const size_t field_tgt_empty = ~(size_t) 0;
+ size_t j, field_tgt_offset = 0, field_tgt_clear = field_tgt_empty;
uintptr_t field_tgt_base = 0;
for (i = 0; i < mapnum; i++)
@@ -841,7 +840,7 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
k->host_end = k->host_start + sizeof (void *);
splay_tree_key n = splay_tree_lookup (mem_map, k);
/* Need to account for the case where a struct field hasn't been
- mapped onto the accelerator yet. */
+ mapped onto the device yet. */
if (n && n->refcount != REFCOUNT_LINK)
gomp_map_vars_existing (devicep, n, k, &tgt->list[i],
kind & typemask, cbufp);
@@ -858,12 +857,12 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
size_t align = (size_t) 1 << (kind >> rshift);
tgt->list[i].key = k;
k->tgt = tgt;
- if (field_tgt_clear != FIELD_TGT_EMPTY)
+ if (field_tgt_clear != field_tgt_empty)
{
k->tgt_offset = k->host_start - field_tgt_base
+ field_tgt_offset;
if (i == field_tgt_clear)
- field_tgt_clear = FIELD_TGT_EMPTY;
+ field_tgt_clear = field_tgt_empty;
}
else
{
--
2.17.1
>From 3bc6c5ad05479678367400431847bf139f661375 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Fri, 21 Dec 2018 11:32:24 +0100
Subject: [PATCH] libgomp/target.c magic constants self-documentation
libgomp/
* libgomp.h (OFFSET_INLINED, OFFSET_POINTER, OFFSET_STRUCT):
Define.
* target.c (gomp_map_val): Use OFFSET_* macros instead of magic
constants. Write as switch instead of list of ifs.
(gomp_map_vars_async): Use OFFSET_* macros. Clarify
field_tgt_clear magic constant.
---
libgomp/libgomp.h | 5 +++++
libgomp/target.c | 49 +++++++++++++++++++++++++++++------------------
2 files changed, 35 insertions(+), 19 deletions(-)
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 8105e640e32d..eef380d7b0fc 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -873,6 +873,11 @@ struct target_var_desc {
uintptr_t length;
};
+/* Special values for struct target_var_desc's offset. */
+#define OFFSET_INLINED (~(uintptr_t) 0)
+#define OFFSET_POINTER (~(uintptr_t) 1)
+#define OFFSET_STRUCT (~(uintptr_t) 2)
+
struct target_mem_desc {
/* Reference count. */
uintptr_t refcount;
diff --git a/libgomp/target.c b/libgomp/target.c
index 0b4e0107f75d..201da567d73a 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -470,17 +470,25 @@ gomp_map_val (struct target_mem_desc *tgt, void **hostaddrs, size_t i)
return tgt->list[i].key->tgt->tgt_start
+ tgt->list[i].key->tgt_offset
+ tgt->list[i].offset;
- if (tgt->list[i].offset == ~(uintptr_t) 0)
- return (uintptr_t) hostaddrs[i];
- if (tgt->list[i].offset == ~(uintptr_t) 1)
- return 0;
- if (tgt->list[i].offset == ~(uintptr_t) 2)
- return tgt->list[i + 1].key->tgt->tgt_start
- + tgt->list[i + 1].key->tgt_offset
- + tgt->list[i + 1].offset
- + (uintptr_t) hostaddrs[i]
- - (uintptr_t) hostaddrs[i + 1];
- return tgt->tgt_start + tgt->list[i].offset;
+
+ switch (tgt->list[i].offset)
+ {
+ case OFFSET_INLINED:
+ return (uintptr_t) hostaddrs[i];
+
+ case OFFSET_POINTER:
+ return 0;
+
+ case OFFSET_STRUCT:
+ return tgt->list[i + 1].key->tgt->tgt_start
+ + tgt->list[i + 1].key->tgt_offset
+ + tgt->list[i + 1].offset
+ + (uintptr_t) hostaddrs[i]
+ - (uintptr_t) hostaddrs[i + 1];
+
+ default:
+ return tgt->tgt_start + tgt->list[i].offset;
+ }
}
attribute_hidden struct target_mem_desc *
@@ -546,7 +554,7 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
|| (kind & typemask) == GOMP_MAP_FIRSTPRIVATE_INT)
{
tgt->list[i].key = NULL;
- tgt->list[i].offset = ~(uintptr_t) 0;
+ tgt->list[i].offset = OFFSET_INLINED;
continue;
}
else if ((kind & typemask) == GOMP_MAP_USE_DEVICE_PTR)
@@ -564,7 +572,7 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
= (void *) (n->tgt->tgt_start + n->tgt_offset
+ cur_node.host_start);
tgt->list[i].key = NULL;
- tgt->list[i].offset = ~(uintptr_t) 0;
+ tgt->list[i].offset = OFFSET_INLINED;
continue;
}
else if ((kind & typemask) == GOMP_MAP_STRUCT)
@@ -575,7 +583,7 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
cur_node.host_end = (uintptr_t) hostaddrs[last]
+ sizes[last];
tgt->list[i].key = NULL;
- tgt->list[i].offset = ~(uintptr_t) 2;
+ tgt->list[i].offset = OFFSET_STRUCT;
splay_tree_key n = splay_tree_lookup (mem_map, &cur_node);
if (n == NULL)
{
@@ -608,7 +616,7 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
else if ((kind & typemask) == GOMP_MAP_ALWAYS_POINTER)
{
tgt->list[i].key = NULL;
- tgt->list[i].offset = ~(uintptr_t) 1;
+ tgt->list[i].offset = OFFSET_POINTER;
has_firstprivate = true;
continue;
}
@@ -638,7 +646,7 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
if (!n)
{
tgt->list[i].key = NULL;
- tgt->list[i].offset = ~(uintptr_t) 1;
+ tgt->list[i].offset = OFFSET_POINTER;
continue;
}
}
@@ -738,7 +746,8 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
if (not_found_cnt)
tgt->array = gomp_malloc (not_found_cnt * sizeof (*tgt->array));
splay_tree_node array = tgt->array;
- size_t j, field_tgt_offset = 0, field_tgt_clear = ~(size_t) 0;
+ const size_t field_tgt_empty = ~(size_t) 0;
+ size_t j, field_tgt_offset = 0, field_tgt_clear = field_tgt_empty;
uintptr_t field_tgt_base = 0;
for (i = 0; i < mapnum; i++)
@@ -830,6 +839,8 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
else
k->host_end = k->host_start + sizeof (void *);
splay_tree_key n = splay_tree_lookup (mem_map, k);
+ /* Need to account for the case where a struct field hasn't been
+ mapped onto the device yet. */
if (n && n->refcount != REFCOUNT_LINK)
gomp_map_vars_existing (devicep, n, k, &tgt->list[i],
kind & typemask, cbufp);
@@ -846,12 +857,12 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
size_t align = (size_t) 1 << (kind >> rshift);
tgt->list[i].key = k;
k->tgt = tgt;
- if (field_tgt_clear != ~(size_t) 0)
+ if (field_tgt_clear != field_tgt_empty)
{
k->tgt_offset = k->host_start - field_tgt_base
+ field_tgt_offset;
if (i == field_tgt_clear)
- field_tgt_clear = ~(size_t) 0;
+ field_tgt_clear = field_tgt_empty;
}
else
{
--
2.17.1
