[Openmp-commits] [PATCH] D77951: [OpenMP] Refined CUDA plugin to put all CUDA operations into class

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:71
   NONE
 };
 
----------------
jdoerfert wrote:
> I imagine the above is all clang-formated, if so split it off, LGTM on that part. (keep the namsepace as part of the other patch)
Okay, got your point.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:89
+std::list<KernelTy> KernelsList;
+
 /// Device environment data
----------------
jdoerfert wrote:
> I don't know where this should go right now but a global list sounds wrong. Follow up.
This is what it is originally. Basically I didn't change any logic in this patch. From its usage, looks like it is only for storage. Items will only be `emplace_back` to it and then get the pointer. That's it.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:117
+  int NumThreads = 0;
+};
 
----------------
jdoerfert wrote:
> 1) Can we replace the list with vectors please. We do not add or delete elements once created.
> 2) Can we make it a template class parametric in the Context type. e.g., `struct NVIDIADeviceDataTy : public DeviceDataTy<CUcontext> {}`.
> 3) These should all be unsigned types, right? We also should add explicit documentation for each. I mean `NumThreads` is some maximum I guess.
> 4) These types need to go into a common header (see also 2)). Follow up though
1. Can we replace the list with vectors please. We do not add or delete elements once created.
Yes, we can do that.
2. Can we make it a template class parametric in the Context type. e.g., `struct NVIDIADeviceDataTy : public DeviceDataTy<CUcontext> {}`.
I don't think so. Now we only know that CUDA has this context thing. We have no idea whether other platforms do. Other things like warp as well.
3. These should all be unsigned types, right? We also should add explicit documentation for each. I mean NumThreads is some maximum I guess.
Yeah, they should be. Will change them correspondingly.
4. These types need to go into a common header (see also 2)). Follow up though.
We should only put the most common part into a common header, but it seems that we only have three known common members: `FuncGblEntries`, `NumTeams`, and `NumThreads`.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:172
     for (std::vector<CUstream> &S : StreamPool)
-      S.resize(EnvNumInitialStreams);
+      S.reserve(EnvNumInitialStreams);
 
----------------
jdoerfert wrote:
> These three lines can be removed if you move the `getenv` stuff into init and call resizeStreamPool there.
Correct. Actually I would leave the getenv stuff here because the env has already been determined during the library is initialized. I mean, the global variable is constructed.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:301
+    E.Table.EntriesBegin = E.Entries.data();
+    E.Table.EntriesEnd = E.Entries.data() + E.Entries.size();
 
----------------
jdoerfert wrote:
> (Off topic: Why do we have a table member that needs to be updated when we use it, that seems wrong.)
Well, that is what it is. We could probably think how to improve it later.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:367
       DP("Parsed OMP_NUM_TEAMS=%d\n", EnvNumTeams);
-    } else {
-      EnvNumTeams = -1;
     }
 
----------------
jdoerfert wrote:
> Style: I'd move the getenv calls into the condition `if (const ... = getenv) `
But here `EnvStr` are used twice...You would like to have two local variables? :-)


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:1044
+  assert(async_info_ptr && "async_info_ptr is nullptr");
+  assert(async_info_ptr->Queue && "async_info_ptr->Queue is nullptr");
 
----------------
jdoerfert wrote:
> I like the validation in the user facing functions. Makes the assertion location better too.
You mean take the validation from `isValidDeviceId` to this function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77951