diff mbox series

libgomp: Device load_image - minor num-funcs/vars check improvement

Message ID aa22572d-e746-464d-8ce6-16657bc64476@baylibre.com
State New
Headers show
Series libgomp: Device load_image - minor num-funcs/vars check improvement | expand

Commit Message

Tobias Burnus Feb. 19, 2024, 12:45 p.m. UTC 
When debugging a linker issue, leading to a mismatch in the number of 
host/device functions, I was surprised by seeing one additional entry. 
Well, it turned out to be due to the ICV variable.

This patch makes it more consistent. The "+1" is returned since 
r12-2769-g0bac793ed6bad2 (for the on-device omp_get_device_num), 
extended in r13-2545-g9f2fca56593a2b for a struct to support more ICV 
variables on the devices [to handle OMP_..._DEV environment variables].

As the value is returned unconditionally, it makes sense to use it both 
for the expected-value diagnostic and for the condition further below.

Comments, suggestions, remarks?

Tobias

PS: Alternative would be to make the plugin's value depend on whether 
the data was loaded. But that would make the number-of-entries assert 
weaker and might cause corner-case issues when a slightly older libgomp 
plugin is used with the updated libgomp.so. Thus, I have settled for the 
attached variant.

Comments

Tobias Burnus Aug. 1, 2024, 8:12 a.m. UTC | #1 
I have sent the following page in February (Stage 4) and didn't want to 
commit it back then. But for Stage 1, it should be fine ... I like to 
commit it tomorrow, unless there are comments suggesting other.

Attached is the unchanged patch and I also added a "diff -w -U1" patch 
as that makes it easier to see the non-re-indent changes.

Tobias

On February 19, 2024, Tobias Burnus wrote:
> When debugging a linker issue, leading to a mismatch in the number of 
> host/device functions, I was surprised by seeing one additional entry. 
> Well, it turned out to be due to the ICV variable.
>
> This patch makes it more consistent. The "+1" is returned since 
> r12-2769-g0bac793ed6bad2 (for the on-device omp_get_device_num), 
> extended in r13-2545-g9f2fca56593a2b for a struct to support more ICV 
> variables on the devices [to handle OMP_..._DEV environment variables].
>
> As the value is returned unconditionally, it makes sense to use it 
> both for the expected-value diagnostic and for the condition further 
> below.
>
> Comments, suggestions, remarks?
>
> Tobias
>
> PS: Alternative would be to make the plugin's value depend on whether 
> the data was loaded. But that would make the number-of-entries assert 
> weaker and might cause corner-case issues when a slightly older 
> libgomp plugin is used with the updated libgomp.so. Thus, I have 
> settled for the attached variant.
diff mbox series

Patch

libgomp: Device load_image - improve minor num-funcs/vars check

The run time library loads the offload functions and variable and optionally
the ICV variable and returns the number of loaded items, which has to match
the host side. The plugin returns "+1" (since GCC 12) for the ICV variable
entry, independently whether it was loaded or not, but the var's value
(start == end == 0) can be used to detect when this failed.

Thus, we can tighten the assert check - which this commit does together with
making the output less surprising - and simplify the condition further below.

libgomp/ChangeLog:

	* plugin/plugin-gcn.c (GOMP_OFFLOAD_load_image): If ICV variable
	is is not available, decrement other_count and thus the return value.
	* plugin/plugin-nvptx.c (GOMP_OFFLOAD_load_image): Likewise.
	* target.c (gomp_load_image_to_device): Extend fatal-error message;
	simplify a condition.

 libgomp/target.c | 78 +++++++++++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 43 deletions(-)

diff --git a/libgomp/target.c b/libgomp/target.c
index 1367e9cce6c..456a9147154 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -2355,15 +2355,14 @@  gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
 				num_ind_funcs
 				  ? (uint64_t *) host_ind_func_table : NULL);
 
-  if (num_target_entries != num_funcs + num_vars
-      /* "+1" due to the additional ICV struct.  */
-      && num_target_entries != num_funcs + num_vars + 1)
+  /* The "+1" is due to the additional ICV struct.  */
+  if (num_target_entries != num_funcs + num_vars + 1)
     {
       gomp_mutex_unlock (&devicep->lock);
       if (is_register_lock)
 	gomp_mutex_unlock (&register_lock);
       gomp_fatal ("Cannot map target functions or variables"
-		  " (expected %u, have %u)", num_funcs + num_vars,
+		  " (expected %u + %u + 1, have %u)", num_funcs, num_vars,
 		  num_target_entries);
     }
 
@@ -2447,48 +2446,41 @@  gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
       array++;
     }
 
-  /* Last entry is for a ICVs variable.
-     Tolerate case where plugin does not return those entries.  */
-  if (num_funcs + num_vars < num_target_entries)
+  /* Last entry is for the ICV struct variable; if absent, start = end = 0.  */
+  struct addr_pair *icv_var = &target_table[num_funcs + num_vars];
+  if (icv_var->start != 0)
     {
-      struct addr_pair *var = &target_table[num_funcs + num_vars];
-
-      /* Start address will be non-zero for the ICVs variable if
-	 the variable was found in this image.  */
-      if (var->start != 0)
+      /* The index of the devicep within devices[] is regarded as its
+	 'device number', which is different from the per-device type
+	 devicep->target_id.  */
+      int dev_num = (int) (devicep - &devices[0]);
+      struct gomp_offload_icvs *icvs = get_gomp_offload_icvs (dev_num);
+      size_t var_size = icv_var->end - icv_var->start;
+      if (var_size != sizeof (struct gomp_offload_icvs))
 	{
-	  /* The index of the devicep within devices[] is regarded as its
-	     'device number', which is different from the per-device type
-	     devicep->target_id.  */
-	  int dev_num = (int) (devicep - &devices[0]);
-	  struct gomp_offload_icvs *icvs = get_gomp_offload_icvs (dev_num);
-	  size_t var_size = var->end - var->start;
-	  if (var_size != sizeof (struct gomp_offload_icvs))
-	    {
-	      gomp_mutex_unlock (&devicep->lock);
-	      if (is_register_lock)
-		gomp_mutex_unlock (&register_lock);
-	      gomp_fatal ("offload plugin managed 'icv struct' not of expected "
-			  "format");
-	    }
-	  /* Copy the ICVs variable to place on device memory, hereby
-	     actually designating its device number into effect.  */
-	  gomp_copy_host2dev (devicep, NULL, (void *) var->start, icvs,
-			      var_size, false, NULL);
-	  splay_tree_key k = &array->key;
-	  k->host_start = (uintptr_t) icvs;
-	  k->host_end =
-	    k->host_start + (size_mask & sizeof (struct gomp_offload_icvs));
-	  k->tgt = tgt;
-	  k->tgt_offset = var->start;
-	  k->refcount = REFCOUNT_INFINITY;
-	  k->dynamic_refcount = 0;
-	  k->aux = NULL;
-	  array->left = NULL;
-	  array->right = NULL;
-	  splay_tree_insert (&devicep->mem_map, array);
-	  array++;
+	  gomp_mutex_unlock (&devicep->lock);
+	  if (is_register_lock)
+	    gomp_mutex_unlock (&register_lock);
+	  gomp_fatal ("offload plugin managed 'icv struct' not of expected "
+		      "format");
 	}
+      /* Copy the ICVs variable to place on device memory, hereby
+	     actually designating its device number into effect.  */
+      gomp_copy_host2dev (devicep, NULL, (void *) icv_var->start, icvs,
+			  var_size, false, NULL);
+      splay_tree_key k = &array->key;
+      k->host_start = (uintptr_t) icvs;
+      k->host_end =
+      k->host_start + (size_mask & sizeof (struct gomp_offload_icvs));
+      k->tgt = tgt;
+      k->tgt_offset = icv_var->start;
+      k->refcount = REFCOUNT_INFINITY;
+      k->dynamic_refcount = 0;
+      k->aux = NULL;
+      array->left = NULL;
+      array->right = NULL;
+      splay_tree_insert (&devicep->mem_map, array);
+      array++;
     }
 
   free (target_table);