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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield 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;
----------------
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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103059