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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:192
+///
+/// `Ty` is the type of the resource. It should be copyable.
+/// `AllocatorTy` should provide two functions:
----------------
What does it mean a resource is copyable? Did you mean a resource handle is copyable?


================
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:
> tianshilei1992 wrote:
> > 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)`.
> Allocator.allocate(Ty&) should return  OFFLOAD_SUCCESS, OFFLOAD_FAIL.
> You have AllocatorTy which knows what Ty is. Indeed, Ty should be removed, it can be derived from AllocatorTy
If Ty can be accessed from AllocatorTy, we'd better avoid passing Ty as a template argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111954