[openmp][amdgpu][nfc] Simplify implicit args handling

[JonChesterfield]

Summary

Removes a +x/-x pair on the only store/load of a variable
and deletes some nearby dead code. Also reduces the size of the implicit
struct to reflect the code currently emitted by clang.

[JonChesterfield]

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

488–489

info.kernel_segment_size is only written here

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

1634

info.kernel_segment_size is only read here

[arsenm]

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

487–489

The naming here is going to be confusing. The compiler reported kernarg size is supposed to be the complete allocation size. If this wants to know the explicit argument size, that should probably go to a different named field

[JonChesterfield]

• delete more

[arsenm]

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

487–489

i.e. nothing about checking hasHiddenArgs makes sense

[JonChesterfield]

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

487–489

It's less confusing after this patch than it was before. Incremental steps. I'm deleting stuff while I work out how this handling is behaving.

[brooks]

openmp/libomptarget/plugins/amdgpu/impl/internal.h

45

This breaks builds on 32-bit platforms (e.g., i386). If they aren't supported that's fine, but then cmake adjustments are required to prevent it from trying to build.

[arsenm]

openmp/libomptarget/plugins/amdgpu/impl/internal.h

45

Just use explicit uint64_t, the named types randomly changing sizes is one of the dumbest things about C

[JonChesterfield]

openmp/libomptarget/plugins/amdgpu/impl/internal.h

45

I don't know if i386 will work in general, but those ^ have to be 64 bit regardless, will fix shortly

[JonChesterfield]

openmp/libomptarget/plugins/amdgpu/impl/internal.h

45

Patch at D116963

openmp/libomptarget/plugins/amdgpu/impl/internal.h

 
 #define MAX_NUM_KERNELS (1024 * 16)
 
 typedef struct impl_implicit_args_s {
   unsigned long offset_x;
   unsigned long offset_y;
   unsigned long offset_z;
   unsigned long hostcall_ptr;
-  char num_gpu_queues;
-  unsigned long gpu_queue_ptr;
-  char num_cpu_queues;
-  unsigned long cpu_worker_signals;
-  unsigned long cpu_queue_ptr;
-  unsigned long kernarg_template_ptr;
+  unsigned long unused0;
+  unsigned long unused1;
+  unsigned long unused2;
 } impl_implicit_args_t;
+static_assert(sizeof(impl_implicit_args_t) == 56, "");
 
 // ---------------------- Kernel Start -------------
 typedef struct atl_kernel_info_s {
   uint64_t kernel_object;
   uint32_t group_segment_size;
   uint32_t private_segment_size;
   uint32_t sgpr_count;
   uint32_t vgpr_count;
   uint32_t sgpr_spill_count;
   uint32_t vgpr_spill_count;
   uint32_t kernel_segment_size;
   uint32_t num_args;
-  std::vector<uint64_t> arg_alignments;
-  std::vector<uint64_t> arg_offsets;
-  std::vector<uint64_t> arg_sizes;
 } atl_kernel_info_t;
 
 typedef struct atl_symbol_info_s {
   uint64_t addr;
   uint32_t size;
 } atl_symbol_info_t;
 
 // ---------------------- Kernel End -------------

openmp/libomptarget/plugins/amdgpu/impl/system.cpp

enum class ValueKind {
     HiddenDefaultQueue,
     HiddenCompletionAction,
     HiddenMultiGridSyncArg,
     HiddenHostcallBuffer,
     Unknown
   };
 
   KernelArgMD()
-      : name_(std::string()), typeName_(std::string()), size_(0), offset_(0),
-        align_(0), valueKind_(ValueKind::Unknown) {}
+      : name_(std::string()),  size_(0), offset_(0),
+        valueKind_(ValueKind::Unknown) {}
 
   // fields
   std::string name_;
-  std::string typeName_;
   uint32_t size_;
   uint32_t offset_;
-  uint32_t align_;
   ValueKind valueKind_;
 };
 
-class KernelMD {
-public:
-  KernelMD() : kernargSegmentSize_(0ull) {}
-
-  // fields
-  uint64_t kernargSegmentSize_;
-};
-
 static const std::map<std::string, KernelArgMD::ValueKind> ArgValueKind = {
     // v3
     //    {"by_value", KernelArgMD::ValueKind::ByValue},
     //    {"global_buffer", KernelArgMD::ValueKind::GlobalBuffer},
     //    {"dynamic_shared_pointer",
     //    KernelArgMD::ValueKind::DynamicSharedPointer},
     //    {"sampler", KernelArgMD::ValueKind::Sampler},
     //    {"image", KernelArgMD::ValueKind::Image},
int populate_kernelArgMD(msgpack::byte_range args_element,
                          KernelArgMD *kernelarg) {
   using namespace msgpack;
   int error = 0;
   foreach_map(args_element, [&](byte_range key, byte_range value) -> void {
     if (message_is_string(key, ".name")) {
       foronly_string(value, [&](size_t N, const unsigned char *str) {
         kernelarg->name_ = std::string(str, str + N);
       });
-    } else if (message_is_string(key, ".type_name")) {
-      foronly_string(value, [&](size_t N, const unsigned char *str) {
-        kernelarg->typeName_ = std::string(str, str + N);
-      });
     } else if (message_is_string(key, ".size")) {
       foronly_unsigned(value, [&](uint64_t x) { kernelarg->size_ = x; });
     } else if (message_is_string(key, ".offset")) {
       foronly_unsigned(value, [&](uint64_t x) { kernelarg->offset_ = x; });
     } else if (message_is_string(key, ".value_kind")) {
       foronly_string(value, [&](size_t N, const unsigned char *str) {
         std::string s = std::string(str, str + N);
         auto itValueKind = ArgValueKind.find(s);
for (size_t i = 0; i < kernelsSize; i++) {
 
     // Make sure that kernelName + ".kd" == symbolName
     if ((kernelName + ".kd") != symbolName) {
       printf("[%s:%d] Kernel name mismatching symbol: %s != %s + .kd\n",
              __FILE__, __LINE__, symbolName.c_str(), kernelName.c_str());
       return HSA_STATUS_ERROR_INVALID_CODE_OBJECT;
     }
 
-    atl_kernel_info_t info = {0, 0, 0, 0, 0, 0, 0, 0, 0, {}, {}, {}};
+    atl_kernel_info_t info = {0, 0, 0, 0, 0, 0, 0, 0, 0};
 
     uint64_t sgpr_count, vgpr_count, sgpr_spill_count, vgpr_spill_count;
     msgpack_errors += map_lookup_uint64_t(element, ".sgpr_count", &sgpr_count);
     if (msgpack_errors != 0) {
       printf("[%s:%d] %s failed\n", __FILE__, __LINE__,
              "sgpr count metadata lookup in kernel metadata");
       return HSA_STATUS_ERROR_INVALID_CODE_OBJECT;
     }
if (kernel_segment_size > 0) {
         }
 
         msgpack_errors += populate_kernelArgMD(args_element, &lcArg);
         if (msgpack_errors != 0) {
           printf("[%s:%d] %s failed\n", __FILE__, __LINE__,
                  "iterate args map in kernel args metadata");
           return HSA_STATUS_ERROR_INVALID_CODE_OBJECT;
         }
-        // populate info with sizes and offsets
-        info.arg_sizes.push_back(lcArg.size_);
         // v3 has offset field and not align field
         size_t new_offset = lcArg.offset_;
         size_t padding = new_offset - offset;
         offset = new_offset;
-        info.arg_offsets.push_back(lcArg.offset_);
         DP("Arg[%lu] \"%s\" (%u, %u)\n", i, lcArg.name_.c_str(), lcArg.size_,
            lcArg.offset_);
         offset += lcArg.size_;
 
         // check if the arg is a hidden/implicit arg
         // this logic assumes that all hidden args are 8-byte aligned
         if (!isImplicit(lcArg.valueKind_)) {
           kernel_explicit_args_size += lcArg.size_;
         } else {
           hasHiddenArgs = true;
         }
         kernel_explicit_args_size += padding;
       }
     }
 
-    // add size of implicit args, e.g.: offset x, y and z and pipe pointer, but
-    // do not count the compiler set implicit args, but set your own implicit
-    // args by discounting the compiler set implicit args
+    // TODO: Probably don't want this arithmetic 
     info.kernel_segment_size =
-        (hasHiddenArgs ? kernel_explicit_args_size : kernel_segment_size) +
-        sizeof(impl_implicit_args_t);
+        (hasHiddenArgs ? kernel_explicit_args_size : kernel_segment_size);
     DP("[%s: kernarg seg size] (%lu --> %u)\n", kernelName.c_str(),
        kernel_segment_size, info.kernel_segment_size);
 
     // kernel received, now add it to the kernel info table
     KernelInfoTable[kernelName] = info;
   }
 
   return HSA_STATUS_SUCCESS;

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

for (__tgt_offload_entry *e = HostBegin; e != HostEnd; ++e) {
     hsa_status_t err = HSA_STATUS_SUCCESS;
     if (!e->name) {
       err = HSA_STATUS_ERROR;
     } else {
       std::string kernelStr = std::string(e->name);
       auto It = KernelInfoMap.find(kernelStr);
       if (It != KernelInfoMap.end()) {
         atl_kernel_info_t info = It->second;
-        // return the size for non-implicit args
-        kernarg_segment_size =
-            info.kernel_segment_size - sizeof(impl_implicit_args_t);
+        kernarg_segment_size = info.kernel_segment_size;
       } else {
         err = HSA_STATUS_ERROR;
       }
     }
 
-    // each arg is a void * in this openmp implementation
-    uint32_t arg_num = kernarg_segment_size / sizeof(void *);
-    std::vector<size_t> arg_sizes(arg_num);
-    for (std::vector<size_t>::iterator it = arg_sizes.begin();
-         it != arg_sizes.end(); it++) {
-      *it = sizeof(void *);
-    }
-
     // default value GENERIC (in case symbol is missing from cubin file)
     llvm::omp::OMPTgtExecModeFlags ExecModeVal =
         llvm::omp::OMPTgtExecModeFlags::OMP_TGT_EXEC_MODE_GENERIC;
 
     // get flat group size if present, else Default_WG_Size
     int16_t WGSizeVal = RTLDeviceInfoTy::Default_WG_Size;
 
     // get Kernel Descriptor if present.