[libc-commits] [PATCH] D152575: Added modf for NVPTX and AMDGPU targets to implement 'libmgpu.a' for math on the GPU

[Joseph Huber via Phabricator via libc-commits]

jhuber6 added a comment.

Remember to clang format via `git clang-format HEAD~1` then `git add -A; git commit --amend` to update if anything was changed.



================
Comment at: libc/src/math/gpu/CMakeLists.txt:47
+  COMPILE_OPTIONS
+    # FIXME: We need a way to pass the flag to only the NVTPX / AMDGPU build.
+    # This shouldn't cause issues because we only link in needed symbols, but it
----------------
Don't need to copy this comment everywhere.


================
Comment at: libc/src/math/gpu/amdgpu/amdgpu.h:18
 namespace __llvm_libc {
 namespace vendor {
 
----------------
This should be `internal` forgot about that when I updated.


================
Comment at: libc/src/math/gpu/amdgpu/amdgpu.h:24
+#pragma omp allocate(tmp) allocator(omp_thread_mem_alloc)
+#endif
+  double r =
----------------
We're not using OpenMP so these won't work nor should they be necessary. Address space five maps to local memory which should be the default here for a variable on the stack. The only reason this was necessary for OpenMP is because of the memory model OpenMP uses trying to mimic the CPU. E.g. the CPU threads can share stack data by default but the GPU can't, so by default we don't put things on the stack in the GPU. Since this is compiled directly for the GPU it should already be stack memory. Ditto the cast shouldn't be necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152575