[libc-commits] [PATCH] D143089: [libc] Remove OpenMP and build the GPU libc directly

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Mostly LGTM but I have left a few nits and request for one TODO. I will approve as soon as I can see the TODO comment.



================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:157
+  # more information.
+  set(packaged_target_name ${fq_target_name}.gpu)
+  set(packaged_output_name ${CMAKE_CURRENT_BINARY_DIR}/${fq_target_name}.bin)
----------------
`gpu` can potentially be directory name. So, can we use a suffix like, `.__gpu__`?


================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:158
+  set(packaged_target_name ${fq_target_name}.gpu)
+  set(packaged_output_name ${CMAKE_CURRENT_BINARY_DIR}/${fq_target_name}.bin)
+  foreach(gpu_target_name ${gpu_target_names})
----------------
Can this be given a suffix of `.gpu.bin`?


================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:163
+    set(input_file $<TARGET_OBJECTS:${gpu_target_name}>)
+    list(APPEND packager_images
+         --image=file=${input_file},arch=${gpu_arch},triple=${gpu_triple})
----------------
Can this be moved into the `foreach` block above?


================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:175
+  # We create an empty 'stub' file for the host to contain the embedded device
+  # code. This will be packaged into 'libcgpu.a'.
+  get_filename_component(stub_filename ${ADD_GPU_ENTRYPOINT_OBJ_SRCS} NAME)
----------------
Can you add a TODO explaining how this will evolve?


================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:204
+  if(TARGET ${internal_target_name})
+    set_target_properties(
+      ${fq_target_name}
----------------
Can this be moved to the `if` block where the internal target is actually added?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143089