[Openmp-commits] [PATCH] D96329: [libomptarget][amdgcn] Tolerate deadstripped env variable

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield created this revision.
JonChesterfield added a reviewer: pdhaliwal.
Herald added subscribers: kerbowa, nhaehnle, jvesely.
JonChesterfield requested review of this revision.
Herald added a project: OpenMP.
Herald added a subscriber: openmp-commits.

[libomptarget][amdgcn] Tolerate deadstripped env variable

Discovered by Pushpinder. If the device_environment variable is unused
it can be deadstripped, in which case we should not abort due to it
missing. This change is safe in that a missing symbol which is actually
used can be reported by both linker and loader, and a missing unused
symbol is better deadstripped than left in the image.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96329

Files:
  openmp/libomptarget/plugins/amdgpu/src/rtl.cpp


Index: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
===================================================================
--- openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
+++ openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
@@ -1025,6 +1025,9 @@
   //  - under review in trunk is debug_level, device_num
   //  - rocmcc matches aomp, patch to swap num_devices and device_num
 
+  // The symbol may also have been deadstripped because the device side
+  // accessors were unused.
+
   // If the symbol is in .data (aomp, rocm) it can be written directly.
   // If it is in .bss, we must wait for it to be allocated space on the
   // gpu (trunk) and initialize after loading.
@@ -1069,39 +1072,43 @@
   bool in_image() { return si.sh_type != SHT_NOBITS; }
 
   atmi_status_t before_loading(void *data, size_t size) {
-    assert(valid);
-    if (in_image()) {
-      DP("Setting global device environment before load (%u bytes)\n", si.size);
-      uint64_t offset = (char *)si.addr - (char *)image->ImageStart;
-      void *pos = (char *)data + offset;
-      memcpy(pos, &host_device_env, si.size);
+    if (valid) {
+      if (in_image()) {
+        DP("Setting global device environment before load (%u bytes)\n",
+           si.size);
+        uint64_t offset = (char *)si.addr - (char *)image->ImageStart;
+        void *pos = (char *)data + offset;
+        memcpy(pos, &host_device_env, si.size);
+      }
     }
     return ATMI_STATUS_SUCCESS;
   }
 
   atmi_status_t after_loading() {
-    assert(valid);
-    if (!in_image()) {
-      DP("Setting global device environment after load (%u bytes)\n", si.size);
-      int device_id = host_device_env.device_num;
+    if (valid) {
+      if (!in_image()) {
+        DP("Setting global device environment after load (%u bytes)\n",
+           si.size);
+        int device_id = host_device_env.device_num;
 
-      void *state_ptr;
-      uint32_t state_ptr_size;
-      atmi_status_t err = atmi_interop_hsa_get_symbol_info(
-          get_gpu_mem_place(device_id), sym(), &state_ptr, &state_ptr_size);
-      if (err != ATMI_STATUS_SUCCESS) {
-        DP("failed to find %s in loaded image\n", sym());
-        return err;
-      }
+        void *state_ptr;
+        uint32_t state_ptr_size;
+        atmi_status_t err = atmi_interop_hsa_get_symbol_info(
+            get_gpu_mem_place(device_id), sym(), &state_ptr, &state_ptr_size);
+        if (err != ATMI_STATUS_SUCCESS) {
+          DP("failed to find %s in loaded image\n", sym());
+          return err;
+        }
 
-      if (state_ptr_size != si.size) {
-        DP("Symbol had size %u before loading, %u after\n", state_ptr_size,
-           si.size);
-        return ATMI_STATUS_ERROR;
-      }
+        if (state_ptr_size != si.size) {
+          DP("Symbol had size %u before loading, %u after\n", state_ptr_size,
+             si.size);
+          return ATMI_STATUS_ERROR;
+        }
 
-      return DeviceInfo.freesignalpool_memcpy_h2d(state_ptr, &host_device_env,
-                                                  state_ptr_size, device_id);
+        return DeviceInfo.freesignalpool_memcpy_h2d(state_ptr, &host_device_env,
+                                                    state_ptr_size, device_id);
+      }
     }
     return ATMI_STATUS_SUCCESS;
   }
@@ -1165,9 +1172,6 @@
   {
     auto env = device_environment(device_id, DeviceInfo.NumberOfDevices, image,
                                   img_size);
-    if (!env.valid) {
-      return NULL;
-    }
 
     atmi_status_t err = module_register_from_memory_to_place(
         (void *)image->ImageStart, img_size, get_gpu_place(device_id),


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D96329.322336.patch
Type: text/x-patch
Size: 3628 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20210209/fd199d76/attachment.bin>