diff mbox

OMP. More constification

Message ID 55B4F635.30400@acm.org
State New
Headers show

Commit Message

Nathan Sidwell July 26, 2015, 3:01 p.m. UTC 
Jakub, Ilya,
I found some more missing consts.  The size, kind, var and function arrays 
emitted by omp-low are read only, but are not so marked.  This patch

a) adds const qualifier and marks them read only.  They now end up in .rodata 
and .data.ro.rel appropriately.

b) adds const qualifier to libgomp's routines that use the data.

The host-side version of the offloaded function takes a 'void *' argument, which 
should really be 'void  *const *', but that change rapidly went to change a lot 
of places.  Even just chaning it to 'const void *'.  So I punted on that bit and 
simply added a 'void *' cast when calling it.

I've not checked the intelmic library, and suspect that will  need some consts 
adding -- is that something you can do Ilya?

nathan

Comments

Jakub Jelinek July 27, 2015, 8:23 a.m. UTC | #1 
On Sun, Jul 26, 2015 at 11:01:09AM -0400, Nathan Sidwell wrote:
> I found some more missing consts.  The size, kind, var and function arrays
> emitted by omp-low are read only, but are not so marked.  This patch

First of all, the hostaddrs array is going to be written by the library
side for GOMP_MAP_FIRSTPRIVATE, so can't be const and shouldn't be const on
the compiler emitted side either.  Similarly for the use_device_ptr clause
on GOMP_target_data* handling.
The size array is const from the library side, sure, but on the compiler
side is only conditionally static (static only if all the values written to
it are INTEGER_CSTs), so not sure if we want to make it use const qualified
type unconditionally, rather than only change it to the const qualified type
if it is TREE_STATIC at the end.
The kinds array I'm afraid might very well soon follow the size array model.

	Jakub
Nathan Sidwell July 27, 2015, 11:33 a.m. UTC | #2 
On 07/27/15 04:23, Jakub Jelinek wrote:
> On Sun, Jul 26, 2015 at 11:01:09AM -0400, Nathan Sidwell wrote:
>> I found some more missing consts.  The size, kind, var and function arrays
>> emitted by omp-low are read only, but are not so marked.  This patch
>
> First of all, the hostaddrs array is going to be written by the library
> side for GOMP_MAP_FIRSTPRIVATE, so can't be const and shouldn't be const on
> the compiler emitted side either.  Similarly for the use_device_ptr clause
> on GOMP_target_data* handling.
> The size array is const from the library side, sure, but on the compiler
> side is only conditionally static (static only if all the values written to
> it are INTEGER_CSTs), so not sure if we want to make it use const qualified
> type unconditionally, rather than only change it to the const qualified type
> if it is TREE_STATIC at the end.
> The kinds array I'm afraid might very well soon follow the size array model.

Ok, thanks for the explanation.  I'm puzzled though about writing the sizes and 
kinds arrays.  They're global object, so that doesn't seem thread-safe?  Or is 
it ill-formed to have multiple host threads execute the same potentially 
offloaded function concurrently?

nathan
Jakub Jelinek July 27, 2015, 11:43 a.m. UTC | #3 
On Mon, Jul 27, 2015 at 07:33:59AM -0400, Nathan Sidwell wrote:
> On 07/27/15 04:23, Jakub Jelinek wrote:
> >On Sun, Jul 26, 2015 at 11:01:09AM -0400, Nathan Sidwell wrote:
> >>I found some more missing consts.  The size, kind, var and function arrays
> >>emitted by omp-low are read only, but are not so marked.  This patch
> >
> >First of all, the hostaddrs array is going to be written by the library
> >side for GOMP_MAP_FIRSTPRIVATE, so can't be const and shouldn't be const on
> >the compiler emitted side either.  Similarly for the use_device_ptr clause
> >on GOMP_target_data* handling.
> >The size array is const from the library side, sure, but on the compiler
> >side is only conditionally static (static only if all the values written to
> >it are INTEGER_CSTs), so not sure if we want to make it use const qualified
> >type unconditionally, rather than only change it to the const qualified type
> >if it is TREE_STATIC at the end.
> >The kinds array I'm afraid might very well soon follow the size array model.
> 
> Ok, thanks for the explanation.  I'm puzzled though about writing the sizes
> and kinds arrays.  They're global object, so that doesn't seem thread-safe?
> Or is it ill-formed to have multiple host threads execute the same
> potentially offloaded function concurrently?

They are only global objects if they are filled with constants.
Right know for kinds array that is true always, for sizes array if no VLAs
nor variable length array sections are used.
They are initially TREE_STATIC (var) = 1; but we later on clear it
with TREE_STATIC (var) = 0; if it is constant (this is still before the
decls are finalized).

	Jakub
Nathan Sidwell July 27, 2015, 11:45 a.m. UTC | #4 
On 07/27/15 07:43, Jakub Jelinek wrote:
> On Mon, Jul 27, 2015 at 07:33:59AM -0400, Nathan Sidwell wrote:

> They are only global objects if they are filled with constants.
> Right know for kinds array that is true always, for sizes array if no VLAs
> nor variable length array sections are used.
> They are initially TREE_STATIC (var) = 1; but we later on clear it
> with TREE_STATIC (var) = 0; if it is constant (this is still before the
> decls are finalized).

Ok, I'd missed that.  thanks.
diff mbox

Patch

2015-07-26  Nathan Sidwell  <nathan@codesourcery.com>

	libgomp/
	* libgomp.h (struct acc_dispatch_t): Constify exec_func's
	object descriptor arguments.
	(gomp_acc_insert_pointer): Constify object descriptor arguments.
	(gomp_map_vars): Likewise.
	* target.c (get_kind, gomp_map_vars, gomp_update, GOMP_target,
	GOMP_target_data, GOMP_target_update): Likewise.
	* oacc-parallel.c (find_pset, GOACC_parallel, GOACC_data_start,
	GOACC_enter_exit_data, GOACC_update): Likewise.
	* libgomp_g.h (GOMP_taeget, GOMP_target_data, GOMP_target_update,
	GOACC_data_start, GOACC_enter_exit_data, GOACC_update): Likewise.
	* plugin/plugin-nvptx.c (nvptx_exec): Likewise.
	* plugin/pligin-host.c (GOMP_OFFLOAD_openacc_parallel): Likewise.
	* oacc-mem.c (gomp_acc_insert_pointer): Likewise.

	gcc/
	* omp-low.c (lower_omp_target): Make size and kind arrays read
	only.
	(omp_finis_file): Make func and var  arrays read only.

Index: libgomp/libgomp.h
===================================================================
--- libgomp/libgomp.h	(revision 226231)
+++ libgomp/libgomp.h	(working copy)
@@ -692,8 +692,9 @@  typedef struct acc_dispatch_t
   struct target_mem_desc *data_environ;
 
   /* Execute.  */
-  void (*exec_func) (void (*) (void *), size_t, void **, void **, size_t *,
-		     unsigned short *, int, int, int, int, void *);
+  void (*exec_func) (void (*) (void *), size_t, void *const *, void *const *,
+		     const size_t *, const unsigned short *,
+		     int, int, int, int, void *);
 
   /* Async cleanup callback registration.  */
   void (*register_async_cleanup_func) (void *);
@@ -771,12 +772,14 @@  struct gomp_device_descr
   acc_dispatch_t openacc;
 };
 
-extern void gomp_acc_insert_pointer (size_t, void **, size_t *, void *);
+extern void gomp_acc_insert_pointer (size_t, void *const *,
+				     const size_t *, const void *);
 extern void gomp_acc_remove_pointer (void *, bool, int, int);
 
 extern struct target_mem_desc *gomp_map_vars (struct gomp_device_descr *,
-					      size_t, void **, void **,
-					      size_t *, void *, bool, bool);
+					      size_t, void *const *, void **,
+					      const size_t *, const void *,
+					      bool, bool);
 extern void gomp_copy_from_async (struct target_mem_desc *);
 extern void gomp_unmap_vars (struct target_mem_desc *, bool);
 extern void gomp_init_device (struct gomp_device_descr *);
Index: libgomp/target.c
===================================================================
--- libgomp/target.c	(revision 226231)
+++ libgomp/target.c	(working copy)
@@ -157,10 +157,10 @@  gomp_map_vars_existing (struct gomp_devi
 }
 
 static int
-get_kind (bool is_openacc, void *kinds, int idx)
+get_kind (bool is_openacc, const void *kinds, int idx)
 {
-  return is_openacc ? ((unsigned short *) kinds)[idx]
-		    : ((unsigned char *) kinds)[idx];
+  return is_openacc ? ((const unsigned short *) kinds)[idx]
+		    : ((const unsigned char *) kinds)[idx];
 }
 
 static void
@@ -219,7 +219,8 @@  gomp_map_pointer (struct target_mem_desc
 
 attribute_hidden struct target_mem_desc *
 gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
-	       void **hostaddrs, void **devaddrs, size_t *sizes, void *kinds,
+	       void *const *hostaddrs, void **devaddrs,
+	       const size_t *sizes, const void *kinds,
 	       bool is_openacc, bool is_target)
 {
   size_t i, tgt_align, tgt_size, not_found_cnt = 0;
@@ -574,8 +575,9 @@  gomp_unmap_vars (struct target_mem_desc
 }
 
 static void
-gomp_update (struct gomp_device_descr *devicep, size_t mapnum, void **hostaddrs,
-	     size_t *sizes, void *kinds, bool is_openacc)
+gomp_update (struct gomp_device_descr *devicep, size_t mapnum,
+	     void *const *hostaddrs,
+	     const size_t *sizes, const void *kinds, bool is_openacc)
 {
   size_t i;
   struct splay_tree_key_s cur_node;
@@ -927,8 +929,8 @@  gomp_fini_device (struct gomp_device_des
 
 void
 GOMP_target (int device, void (*fn) (void *), const void *unused,
-	     size_t mapnum, void **hostaddrs, size_t *sizes,
-	     unsigned char *kinds)
+	     size_t mapnum, void *const *hostaddrs, const size_t *sizes,
+	     const unsigned char *kinds)
 {
   struct gomp_device_descr *devicep = resolve_device (device);
 
@@ -944,7 +946,7 @@  GOMP_target (int device, void (*fn) (voi
 	  thr->place = old_thr.place;
 	  thr->ts.place_partition_len = gomp_places_list_len;
 	}
-      fn (hostaddrs);
+      fn ((void *)hostaddrs);
       gomp_free_thread (thr);
       *thr = old_thr;
       return;
@@ -995,7 +997,8 @@  GOMP_target (int device, void (*fn) (voi
 
 void
 GOMP_target_data (int device, const void *unused, size_t mapnum,
-		  void **hostaddrs, size_t *sizes, unsigned char *kinds)
+		  void *const *hostaddrs, const size_t *sizes,
+		  const unsigned char *kinds)
 {
   struct gomp_device_descr *devicep = resolve_device (device);
 
@@ -1045,7 +1048,8 @@  GOMP_target_end_data (void)
 
 void
 GOMP_target_update (int device, const void *unused, size_t mapnum,
-		    void **hostaddrs, size_t *sizes, unsigned char *kinds)
+		    void *const *hostaddrs, const size_t *sizes,
+		    const unsigned char *kinds)
 {
   struct gomp_device_descr *devicep = resolve_device (device);
 
Index: libgomp/oacc-parallel.c
===================================================================
--- libgomp/oacc-parallel.c	(revision 226231)
+++ libgomp/oacc-parallel.c	(working copy)
@@ -39,7 +39,7 @@ 
 #include <assert.h>
 
 static int
-find_pset (int pos, size_t mapnum, unsigned short *kinds)
+find_pset (int pos, size_t mapnum, const unsigned short *kinds)
 {
   if (pos + 1 >= mapnum)
     return 0;
@@ -53,8 +53,8 @@  static void goacc_wait (int async, int n
 
 void
 GOACC_parallel (int device, void (*fn) (void *),
-		size_t mapnum, void **hostaddrs, size_t *sizes,
-		unsigned short *kinds,
+		size_t mapnum, void *const *hostaddrs, const size_t *sizes,
+		const unsigned short *kinds,
 		int num_gangs, int num_workers, int vector_length,
 		int async, int num_waits, ...)
 {
@@ -95,13 +95,13 @@  GOACC_parallel (int device, void (*fn) (
   if (host_fallback)
     {
       goacc_save_and_set_bind (acc_device_host);
-      fn (hostaddrs);
+      fn ((void *)hostaddrs);
       goacc_restore_bind ();
       return;
     }
   else if (acc_device_type (acc_dev->type) == acc_device_host)
     {
-      fn (hostaddrs);
+      fn ((void *)hostaddrs);
       return;
     }
 
@@ -155,8 +155,8 @@  GOACC_parallel (int device, void (*fn) (
 }
 
 void
-GOACC_data_start (int device, size_t mapnum,
-		  void **hostaddrs, size_t *sizes, unsigned short *kinds)
+GOACC_data_start (int device, size_t mapnum, void *const *hostaddrs,
+		  const size_t *sizes, const unsigned short *kinds)
 {
   bool host_fallback = device == GOMP_DEVICE_HOST_FALLBACK;
   struct target_mem_desc *tgt;
@@ -206,8 +206,8 @@  GOACC_data_end (void)
 }
 
 void
-GOACC_enter_exit_data (int device, size_t mapnum,
-		       void **hostaddrs, size_t *sizes, unsigned short *kinds,
+GOACC_enter_exit_data (int device, size_t mapnum, void *const *hostaddrs,
+		       const size_t *sizes, const unsigned short *kinds,
 		       int async, int num_waits, ...)
 {
   struct goacc_thread *thr;
@@ -368,8 +368,8 @@  goacc_wait (int async, int num_waits, va
 }
 
 void
-GOACC_update (int device, size_t mapnum,
-	      void **hostaddrs, size_t *sizes, unsigned short *kinds,
+GOACC_update (int device, size_t mapnum, void *const *hostaddrs,
+	      const size_t *sizes, const unsigned short *kinds,
 	      int async, int num_waits, ...)
 {
   bool host_fallback = device == GOMP_DEVICE_HOST_FALLBACK;
Index: libgomp/libgomp_g.h
===================================================================
--- libgomp/libgomp_g.h	(revision 226231)
+++ libgomp/libgomp_g.h	(working copy)
@@ -206,27 +206,31 @@  extern void GOMP_single_copy_end (void *
 
 /* target.c */
 
-extern void GOMP_target (int, void (*) (void *), const void *,
-			 size_t, void **, size_t *, unsigned char *);
-extern void GOMP_target_data (int, const void *,
-			      size_t, void **, size_t *, unsigned char *);
+extern void GOMP_target (int, void (*) (void *), const void *, size_t,
+			 void *const *, const size_t *, const unsigned char *);
+extern void GOMP_target_data (int, const void *, size_t,
+			      void *const *, const size_t *,
+			      const unsigned char *);
 extern void GOMP_target_end_data (void);
-extern void GOMP_target_update (int, const void *,
-				size_t, void **, size_t *, unsigned char *);
+extern void GOMP_target_update (int, const void *, size_t,
+				void *const *, const size_t *,
+				const unsigned char *);
 extern void GOMP_teams (unsigned int, unsigned int);
 
 /* oacc-parallel.c */
 
-extern void GOACC_data_start (int, size_t, void **, size_t *,
-			      unsigned short *);
+extern void GOACC_data_start (int, size_t, void *const *, const size_t *,
+			      const unsigned short *);
 extern void GOACC_data_end (void);
-extern void GOACC_enter_exit_data (int, size_t, void **,
-				   size_t *, unsigned short *, int, int, ...);
+extern void GOACC_enter_exit_data (int, size_t, void *const *,
+				   const size_t *, const unsigned short *,
+				   int, int, ...);
 extern void GOACC_parallel (int, void (*) (void *), size_t,
-			    void **, size_t *, unsigned short *, int, int, int,
+			    void *const *, const size_t *,
+			    const unsigned short *, int, int, int,
 			    int, int, ...);
-extern void GOACC_update (int, size_t, void **, size_t *,
-			  unsigned short *, int, int, ...);
+extern void GOACC_update (int, size_t, void *const *, const size_t *,
+			  const unsigned short *, int, int, ...);
 extern void GOACC_wait (int, int, ...);
 extern int GOACC_get_num_threads (void);
 extern int GOACC_get_thread_num (void);
Index: libgomp/plugin/plugin-nvptx.c
===================================================================
--- libgomp/plugin/plugin-nvptx.c	(revision 226231)
+++ libgomp/plugin/plugin-nvptx.c	(working copy)
@@ -1001,9 +1001,11 @@  event_add (enum ptx_event_type type, CUe
 }
 
 void
-nvptx_exec (void (*fn), size_t mapnum, void **hostaddrs, void **devaddrs,
-	  size_t *sizes, unsigned short *kinds, int num_gangs, int num_workers,
-	  int vector_length, int async, void *targ_mem_desc)
+nvptx_exec (void (*fn), size_t mapnum,
+	    void *const *hostaddrs, void *const *devaddrs,
+	    const size_t *sizes, const unsigned short *kinds,
+	    int num_gangs, int num_workers, int vector_length,
+	    int async, void *targ_mem_desc)
 {
   struct targ_fn_descriptor *targ_fn = (struct targ_fn_descriptor *) fn;
   CUfunction function;
Index: libgomp/plugin/plugin-host.c
===================================================================
--- libgomp/plugin/plugin-host.c	(revision 226231)
+++ libgomp/plugin/plugin-host.c	(working copy)
@@ -168,10 +168,10 @@  GOMP_OFFLOAD_run (int n __attribute__ ((
 STATIC void
 GOMP_OFFLOAD_openacc_parallel (void (*fn) (void *),
 			       size_t mapnum __attribute__ ((unused)),
-			       void **hostaddrs __attribute__ ((unused)),
-			       void **devaddrs __attribute__ ((unused)),
-			       size_t *sizes __attribute__ ((unused)),
-			       unsigned short *kinds __attribute__ ((unused)),
+			       void *const *hostaddrs __attribute__ ((unused)),
+			       void *const *devaddrs __attribute__ ((unused)),
+			       const size_t *sizes __attribute__ ((unused)),
+			       const unsigned short *kinds __attribute__ ((unused)),
 			       int num_gangs __attribute__ ((unused)),
 			       int num_workers __attribute__ ((unused)),
 			       int vector_length __attribute__ ((unused)),
@@ -181,10 +181,10 @@  GOMP_OFFLOAD_openacc_parallel (void (*fn
 #ifdef HOST_NONSHM_PLUGIN
   struct nonshm_thread *thd = GOMP_PLUGIN_acc_thread ();
   thd->nonshm_exec = true;
-  fn (devaddrs);
+  fn ((void *)devaddrs);
   thd->nonshm_exec = false;
 #else
-  fn (hostaddrs);
+  fn ((void *)hostaddrs);
 #endif
 }
 
Index: libgomp/oacc-mem.c
===================================================================
--- libgomp/oacc-mem.c	(revision 226231)
+++ libgomp/oacc-mem.c	(working copy)
@@ -585,8 +585,8 @@  acc_update_self (void *h, size_t s)
 }
 
 void
-gomp_acc_insert_pointer (size_t mapnum, void **hostaddrs, size_t *sizes,
-			 void *kinds)
+gomp_acc_insert_pointer (size_t mapnum, void *const *hostaddrs,
+			 const size_t *sizes, const void *kinds)
 {
   struct target_mem_desc *tgt;
   struct goacc_thread *thr = goacc_thread ();
Index: gcc/omp-low.c
===================================================================
--- gcc/omp-low.c	(revision 226231)
+++ gcc/omp-low.c	(working copy)
@@ -11359,12 +11359,16 @@  lower_omp_target (gimple_stmt_iterator *
       TREE_ADDRESSABLE (ctx->sender_decl) = 1;
       t = make_tree_vec (3);
       TREE_VEC_ELT (t, 0) = ctx->sender_decl;
-      TREE_VEC_ELT (t, 1)
-	= create_tmp_var (build_array_type_nelts (size_type_node, map_cnt),
+      tree cst_size = build_qualified_type (size_type_node, TYPE_QUAL_CONST);
+      tree size_var
+	= create_tmp_var (build_array_type_nelts (cst_size, map_cnt),
 			  ".omp_data_sizes");
-      DECL_NAMELESS (TREE_VEC_ELT (t, 1)) = 1;
-      TREE_ADDRESSABLE (TREE_VEC_ELT (t, 1)) = 1;
-      TREE_STATIC (TREE_VEC_ELT (t, 1)) = 1;
+      DECL_NAMELESS (size_var) = 1;
+      TREE_ADDRESSABLE (size_var) = 1;
+      TREE_STATIC (size_var) = 1;
+      TREE_READONLY (size_var) = 1;
+      TREE_VEC_ELT (t, 1) = size_var;
+      
       tree tkind_type;
       int talign_shift;
       if (is_gimple_omp_oacc (stmt))
@@ -11377,12 +11381,15 @@  lower_omp_target (gimple_stmt_iterator *
 	  tkind_type = unsigned_char_type_node;
 	  talign_shift = 3;
 	}
-      TREE_VEC_ELT (t, 2)
+      tkind_type = build_qualified_type (tkind_type, TYPE_QUAL_CONST);
+      tree kind_var
 	= create_tmp_var (build_array_type_nelts (tkind_type, map_cnt),
 			  ".omp_data_kinds");
-      DECL_NAMELESS (TREE_VEC_ELT (t, 2)) = 1;
-      TREE_ADDRESSABLE (TREE_VEC_ELT (t, 2)) = 1;
-      TREE_STATIC (TREE_VEC_ELT (t, 2)) = 1;
+      DECL_NAMELESS (kind_var) = 1;
+      TREE_ADDRESSABLE (kind_var) = 1;
+      TREE_STATIC (kind_var) = 1;
+      TREE_READONLY (kind_var) = 1;
+      TREE_VEC_ELT (t, 2) = kind_var;
       gimple_omp_target_set_data_arg (stmt, t);
 
       vec<constructor_elt, va_gc> *vsize;
@@ -13778,14 +13785,14 @@  omp_finish_file (void)
       vec<constructor_elt, va_gc> *v_f, *v_v;
       vec_alloc (v_f, num_funcs);
       vec_alloc (v_v, num_vars * 2);
-
+      
       add_decls_addresses_to_decl_constructor (offload_funcs, v_f);
       add_decls_addresses_to_decl_constructor (offload_vars, v_v);
 
-      tree vars_decl_type = build_array_type_nelts (pointer_sized_int_node,
-						    num_vars * 2);
-      tree funcs_decl_type = build_array_type_nelts (pointer_sized_int_node,
-						     num_funcs);
+      tree cst_ptr = build_qualified_type (pointer_sized_int_node,
+					   TYPE_QUAL_CONST);
+      tree vars_decl_type = build_array_type_nelts (cst_ptr, num_vars * 2);
+      tree funcs_decl_type = build_array_type_nelts (cst_ptr, num_funcs);
       TYPE_ALIGN (vars_decl_type) = TYPE_ALIGN (pointer_sized_int_node);
       TYPE_ALIGN (funcs_decl_type) = TYPE_ALIGN (pointer_sized_int_node);
       tree ctor_v = build_constructor (vars_decl_type, v_v);
@@ -13799,6 +13806,7 @@  omp_finish_file (void)
 				   get_identifier (".offload_var_table"),
 				   vars_decl_type);
       TREE_STATIC (funcs_decl) = TREE_STATIC (vars_decl) = 1;
+      TREE_READONLY (funcs_decl) = TREE_READONLY (vars_decl) = 1;
       /* Do not align tables more than TYPE_ALIGN (pointer_sized_int_node),
 	 otherwise a joint table in a binary will contain padding between
 	 tables from multiple object files.  */