[Openmp-commits] [PATCH] D103059: [libomptarget] [amdgpu] Added LDS usage to the kernel trace

[Dhruva Chakrabarti via Phabricator via Openmp-commits]

dhruvachak added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1791
 
+  uint32_t group_segment_size;
   uint32_t sgpr_count, vgpr_count, sgpr_spill_count, vgpr_spill_count;
----------------
JonChesterfield wrote:
> 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
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.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1862
     {
       auto it = KernelInfoTable[device_id][kernel_name];
       packet->kernel_object = it.kernel_object;
----------------
JonChesterfield wrote:
> 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
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103059/new/

https://reviews.llvm.org/D103059