[Openmp-commits] [PATCH] D105239: [libomptarget][nfc] Group environment variables, drop accesses to DeviceInfo global

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2098
   if (print_kernel_trace >= LAUNCH) {
+    int num_groups = GridSize / WorkgroupSize;
     // enum modes are SPMD, GENERIC, NONE 0,1,2
----------------
JonChesterfield wrote:
> dhruvachak wrote:
> > Since we already computed num_groups within getLaunchVals, should we return that as well? Otherwise, we have duplicate computation of num_groups and they may go out of sync. Basically, the structure launchVals can have 3 fields including NumGroups. 
> It isn't used for anything aside from printing, and the printing will need to change if we start using a value that isn't a multiple of workgroup size anyway.
> 
> Adding a redundant third return field that is ~unused would make the function harder to test. Suggest changing the print statement if it's the division you'd like to remove.
Oh, and this part is also in the smaller patch proposed before this one. I was hoping we could review the base first, then rebase this for better signal to noise. I can't remember how to build stacked diffs in phab 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105239