[libc-commits] [PATCH] D146681: [libc] Add a loader utility for NVPTX architectures for testing

[Jon Chesterfield via Phabricator via libc-commits]

JonChesterfield added a comment.

Error handling could be better but otherwise looks ok here



================
Comment at: libc/utils/gpu/loader/Loader.h:21
+/// Copy the system's argument vector to GPU memory allocated using \p alloc.
+template <typename Allocator>
+void *copy_argument_vector(int argc, char **argv, Allocator alloc) {
----------------
I'd expect this to catch returning 0/null and propagate that from the interface. Shared memory feels more likely to run out than address space.

Might be worth rewriting this to a single alloc - on HSA each of those allocations will be rounded up to a multiple of 4k internally.

Doing both would mean we can return null on failure without have to pass a deallocator along for the failure path.

Not blocking at this time though, it's probably difficult to hit that exhaustion from libc startup.


================
Comment at: libc/utils/gpu/loader/amdgpu/Loader.cpp:284
+    void *dev_ptr = nullptr;
+    hsa_amd_memory_pool_allocate(finegrained_pool, size,
+                                 /*flags=*/0, &dev_ptr);
----------------
This probably should check the return codes


================
Comment at: libc/utils/gpu/loader/nvptx/Loader.cpp:83
+    if (CUresult err = cuMemAllocHost(&dev_ptr, size))
+      handle_error(err);
+    return dev_ptr;
----------------
That's inconsistent with ignoring errors on the other path 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146681