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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

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 <https://reviews.llvm.org/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.



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1862
     {
       auto it = KernelInfoTable[device_id][kernel_name];
       packet->kernel_object = it.kernel_object;
----------------
dhruvachak wrote:
> 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. 
Yes, but the separate scope is just braces. I was suggesting removing the braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103059