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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked 13 inline comments as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:117
+  int NumThreads = 0;
+};
 
----------------
jdoerfert wrote:
> tianshilei1992 wrote:
> > 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`.
> If we need a "common/gpu" header we can do that as well. AMD will at least look similar enough to reuse the same code. They have some "context" even if it is a class we define in the AMD headers. You can even go as far and make
> 
> `common/Device.h` with the entries you mentioned and then `common/GPUDevice.h` with the template class `template<typename CtxTy> GPUDeviceDataTy : public DeviceDataTy`.
This part is worth a new patch. Let's do it in next step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77951