[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:1874
 };
-
-launchVals getLaunchVals(int ConstWGSize, int ExecutionMode, int EnvTeamLimit,
-                         int EnvNumTeams, int num_teams, int thread_limit,
+launchVals getLaunchVals(EnvironmentVariables Env, int ConstWGSize,
+                         int ExecutionMode, int num_teams, int thread_limit,
----------------
dhruvachak wrote:
> Since we are cleaning up, let's use the same style for all parameters, e.g. CamelCase.
The parameter rename is in D105237. Renaming the parameters will cause a lot of delta in the function body, making it much harder to review the semantic change, do I'd rather do the rename separately.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105239