[Openmp-commits] [PATCH] D111954: [OpenMP][Plugin] Introduce resouce manager

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:213
+    for (size_t I = CurSize; I < Size; ++I)
+      Resources.push_back(Allocator.allocate());
   }
----------------
ye-luo wrote:
> Whe CUDA call fails. Allocator.allocate() returns nullptr and the code continues running. This doesn't make sense.
`ResourceManagerTy` doesn't know what is a valid return value. `Ty` can be pointer, object, etc. Of course, for CUDA, everything is pointer, but it doesn't sound right to have if (Ty == nullptr)`.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:270
+
+  CUstream allocate() noexcept {
+    if (!checkResult(cuCtxSetCurrent(DeviceData.Context),
----------------
ye-luo wrote:
> ye-luo wrote:
> > allocate and deallocate are really meant for pointers. How about create/destroy for single object.
> Error handling and returning the object needs to be separate.
`create` and `destroy` sound good.

> Error handling and returning the object needs to be separate.
I didn't follow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111954