[libomptarget] [amdgpu] Added LDS usage to the kernel trace

[JonChesterfield]

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

1791

would you mind drive-by fixing the declare - brace - mutate construct while you're in the area? I.e. replace the nested block with

const auto kernelInfo = KernelInfoTable[device_id][kernel_name];
const uint32_t group_segment_size = kernelInfo.group_segment_size;
const uint32_t sgpr_count = kernelInfo.sgpr_count;
...

or just use kernelInfo.group_segment_size etc directly, not sure the local name helps readability much

1862

this is a bit odd - have already done the lookup, and stored the fields in local variables, that could be written directly into the packet-> fields above

[JonChesterfield]

no objection to the functional change, would be good to refactor the local weirdness in passing

[dhruvachak]

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

1791

I agree that we can use some cleanup here. I intend to use some of these variables in computing the launch values. Let's revisit the cleanup after that change.

1862

Because the iterator is in a different scope, it is gone, so needs to do a lookup again. Let's do all these cleanups in a different commit.

[JonChesterfield]

Refactoring separately is good, but only works in practice if it's done first. Otherwise the refactor usually gets forgotten, or replaced with something higher priority, and that is the path to entropy.

E.g. D103037 had a suggestion for eliminating the class of typos it was fixing, but as it wasn't done first or simultaneously, there's a reasonable chance it's been forgotten in favour of this line of patches.

Please do the cleanups before landing the associated patch, or inline with it if they are small.

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

1862

Yes, but the separate scope is just braces. I was suggesting removing the braces.

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

int32_t __tgt_rtl_run_target_team_region_locked(
 
   std::string kernel_name = std::string(KernelInfo->Name);
   if (KernelInfoTable[device_id].find(kernel_name) ==
       KernelInfoTable[device_id].end()) {
     DP("Kernel %s not found\n", kernel_name.c_str());
     return OFFLOAD_FAIL;
   }
 
+  uint32_t group_segment_size;
   uint32_t sgpr_count, vgpr_count, sgpr_spill_count, vgpr_spill_count;
 
   {
     auto it = KernelInfoTable[device_id][kernel_name];
+    group_segment_size = it.group_segment_size;
     sgpr_count = it.sgpr_count;
     vgpr_count = it.vgpr_count;
     sgpr_spill_count = it.sgpr_spill_count;
     vgpr_spill_count = it.vgpr_spill_count;
   }
 
   /*
    * Set limit based on ThreadsPerGroup and GroupsPerDevice
getLaunchVals(threadsPerGroup, num_groups, KernelInfo->ConstWGSize,
                 KernelInfo->device_id);
 
   if (print_kernel_trace >= LAUNCH) {
     // enum modes are SPMD, GENERIC, NONE 0,1,2
     // if doing rtl timing, print to stderr, unless stdout requested.
     bool traceToStdout = print_kernel_trace & (RTL_TO_STDOUT | RTL_TIMING);
     fprintf(traceToStdout ? stdout : stderr,
             "DEVID:%2d SGN:%1d ConstWGSize:%-4d args:%2d teamsXthrds:(%4dX%4d) "
-            "reqd:(%4dX%4d) sgpr_count:%u vgpr_count:%u sgpr_spill_count:%u "
-            "vgpr_spill_count:%u tripcount:%lu n:%s\n",
+            "reqd:(%4dX%4d) lds_usage:%uB sgpr_count:%u vgpr_count:%u "
+            "sgpr_spill_count:%u vgpr_spill_count:%u tripcount:%lu n:%s\n",
             device_id, KernelInfo->ExecutionMode, KernelInfo->ConstWGSize,
             arg_num, num_groups, threadsPerGroup, num_teams, thread_limit,
-            sgpr_count, vgpr_count, sgpr_spill_count, vgpr_spill_count,
-            loop_tripcount, KernelInfo->Name);
+            group_segment_size, sgpr_count, vgpr_count, sgpr_spill_count,
+            vgpr_spill_count, loop_tripcount, KernelInfo->Name);
   }
 
   // Run on the device.
   {
     hsa_queue_t *queue = DeviceInfo.HSAQueues[device_id];
     if (!queue) {
       return OFFLOAD_FAIL;
     }
// Run on the device.
     packet->private_segment_size = 0;
     packet->group_segment_size = 0;
     packet->kernel_object = 0;
     packet->kernarg_address = 0;     // use the block allocator
     packet->reserved2 = 0;           // atmi writes id_ here
     packet->completion_signal = {0}; // may want a pool of signals
 
     {
       auto it = KernelInfoTable[device_id][kernel_name];
       packet->kernel_object = it.kernel_object;
       packet->private_segment_size = it.private_segment_size;
       packet->group_segment_size = it.group_segment_size;
       assert(arg_num == (int)it.num_args);
     }
 
     KernelArgPool *ArgPool = nullptr;
     {