diff mbox series

libgomp/target.c magic constants self-documentation

Message ID 871s6bqi0s.fsf@euler.schwinge.homeip.net
State New
Headers show
Series libgomp/target.c magic constants self-documentation | expand

Commit Message

Thomas Schwinge Dec. 21, 2018, 10:41 a.m. UTC 
Hi!

On Sat, 10 Nov 2018 09:11:18 -0800, Julian Brown <julian@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

Comments

Thomas Schwinge May 29, 2019, 2:48 p.m. UTC | #1 
Hi Jakub!

Ping.

On Fri, 21 Dec 2018 11:41:07 +0100, I wrote:
> On Sat, 10 Nov 2018 09:11:18 -0800, Julian Brown <julian@codesourcery.com> wrote:
> > This patch [...] 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
diff mbox series

Patch

From 3bc6c5ad05479678367400431847bf139f661375 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@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