[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:213
+    for (size_t I = CurSize; I < Size; ++I)
+      Resources.push_back(Allocator.allocate());
   }
----------------
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


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:270
+
+  CUstream allocate() noexcept {
+    if (!checkResult(cuCtxSetCurrent(DeviceData.Context),
----------------
tianshilei1992 wrote:
> 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.
I mean `int32_t Allocator.create(Ty& res)`
So the return is OFFLOAD_SUCCESS or OFFLOAD_FAIL.
The created resource is returned via res.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111954