[CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

[tra]

Summary

The file must go under cuda_wrappers/bits/, but was copied
directly into cuda_wrappers/ during installation.

https://github.com/llvm/llvm-project/issues/62939

[qiongsiwu1]

clang/lib/Headers/CMakeLists.txt

519–525

Do we need an install target for ${cuda_wrapper_bits_files} for the cuda-resource-headers component as well? It seems to be the case because this patch is treating ${cuda_wrapper_bits_files} as part of cuda-resource-headers.

add_header_target("cuda-resource-headers" "${cuda_files};${cuda_wrapper_files};${cuda_wrapper_bits_files}")

[tra]

clang/lib/Headers/CMakeLists.txt

519–525

I'm not sure I understand the question. Are you saying that a separate install() for the 'bits' is not necessary and we could just install all headers with a single install above?

If that's the case, then, AFAICT, the answer is that we do need a separate install.
install(FILES) does not preserve the directory structure and dumps all files listed in FILES, regardless if they are in different directories into the same DESTINATION directory.
That is exactly the problem this patch is intended to fix. We do need to place the file under cuda_wrappers/bits/ directory and that's why we have separate install(DESTINATION ${header_install_dir}/cuda_wrappers/bits) here.

install(DIRECTORY) would presumably preserve the source directory structure, but we lose per-file granularity. It may work for the files under cuda_wrappers for now, but I think there's some merit in explicitly controlling which headers we ship and where we put them. While we do have 1:1 mapping between the source tree and install tree, it may not always be the case.

[qiongsiwu1]

clang/lib/Headers/CMakeLists.txt

519–525

Ah sorry for the confusion.

Are you saying that a separate install() for the 'bits' is not necessary and we could just install all headers with a single install above?

No I am trying to say the opposite. I am suggesting we add the separate install target as a component of clang-resource-headers and as a component of cuda-resource-headers, as shown in the code change suggested in the comment above. I am not suggesting any code form this patch to be removed. The cuda-resource-headers can be used to install the cuda related headers only, in the case when a user do not want to install all the headers (e.g. if a user only want to install support for Intel and Nvidia headers, but not the PowerPC headers, the user can select core-resource-headers, x86_files and cuda-resource-headers during a distribution build/install). I think without the code change suggested above, if a user select to install cuda-resource-headers only without specifying clang-resource-headers, we will miss the file cuda_wrappers/bits/shared_ptr_base.h.

[qiongsiwu1]

clang/lib/Headers/CMakeLists.txt

519–525

Sorry I made a typo in the previous comment. I meant x86-resource-headers when I said x86_files.

[tra]

Verified that install works correctly with
individual component installations:

cmake -DCOMPONENT=cuda-resource-headers -P ./cmake_install.cmake
cmake -DCOMPONENT=clang-resource-headers -P ./cmake_install.cmake

[tra]

clang/lib/Headers/CMakeLists.txt

519–525

I think understand now.
cmake -DCOMPONENT=cuda-resource-headers -P ./cmake_install.cmake indeed does not install the bits component.

I've added the install with COMPONENT clang-resource-headers and verified that the bits header is installed during individual component installation.

[qiongsiwu1]

clang/lib/Headers/CMakeLists.txt

519–525

Thanks for the changes! One thing I realized after seeing the most recent update is that I mixed up the term "install target" with "install rules" in my previous comments. Apologies for the confusion.

I think we will need the following new target code only if we intend to specify cuda-resource-bits-headers as a separate installation target (e.g. if we do cmake -DCOMPONENT=cuda-resource-bits-headers -P ./cmake_install.cmake).

add_header_target("cuda-resource-bits-headers" "${cuda_wrapper_bits_files}")
add_dependencies("clang-resource-headers"
                  ... 
                  "cuda-resource-bits-headers"
                 ...
add_llvm_install_targets(install-cuda-resource-bits-headers
                         DEPENDS cuda-resource-bits-headers
                        COMPONENT cuda-resource-headers)

In other words, we do not need these three code changes above if we do not intend to install cuda-resource-bits-headers as a standalone target. If our intention is to always use it as a component of cuda-resource-headers, adding install rule

install(
   FILES ${cuda_wrapper_bits_files}
   DESTINATION ${header_install_dir}/cuda_wrappers/bits
   EXCLUDE_FROM_ALL
   COMPONENT cuda-resource-headers)

should be sufficient when installing cuda-resource-headers by itself. We still need the install rule for clang-resource-headers which was initially added and is still necessary. I tested the installation without the extra targets but with the rule and it seems to work as intended.

My suggestion is that we can remove the new targets if we do not intend to install cuda-resource-bits-headers as a standalone component.

[tra]

Updated according to comments.

[tra]

@qiongsiwu1 : I've updated the patch. PTAL.

[qiongsiwu1]

In D151503#4381548, @tra wrote:

@qiongsiwu1 : I've updated the patch. PTAL.

LGTM! Thank you!

clang/lib/Headers/CMakeLists.txt

set(files
   ${utility_files}
   )
 
 set(cuda_wrapper_files
   cuda_wrappers/algorithm
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
 set(ppc_wrapper_files
   ppc_wrappers/mmintrin.h
   ppc_wrappers/xmmintrin.h
   ppc_wrappers/mm_malloc.h
   ppc_wrappers/emmintrin.h
function(clang_generate_header td_option td_file out_file)
   copy_header_to_output_dir(${CMAKE_CURRENT_BINARY_DIR} ${out_file})
   set(out_files ${out_files} PARENT_SCOPE)
   list(APPEND generated_files "${CMAKE_CURRENT_BINARY_DIR}/${out_file}")
   set(generated_files ${generated_files} PARENT_SCOPE)
 endfunction(clang_generate_header)
 
 
 # Copy header files from the source directory to the build directory
-foreach( f ${files} ${cuda_wrapper_files} ${ppc_wrapper_files} ${openmp_wrapper_files} ${hlsl_files})
+foreach( f ${files} ${cuda_wrapper_files} ${cuda_wrapper_bits_files}
+           ${ppc_wrapper_files} ${openmp_wrapper_files} ${hlsl_files})
   copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR} ${f})
 endforeach( f )
 
 # Generate header files and copy them to the build directory
 if(ARM IN_LIST LLVM_TARGETS_TO_BUILD OR AArch64 IN_LIST LLVM_TARGETS_TO_BUILD)
   # Generate arm_neon.h
   clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
   # Generate arm_fp16.h

 
 # Core/common headers
 add_header_target("core-resource-headers" ${core_files})
 add_header_target("arm-common-resource-headers" "${arm_common_files};${arm_common_generated_files}")
 
 # Architecture/platform specific targets
 add_header_target("arm-resource-headers" "${arm_only_files};${arm_only_generated_files}")
 add_header_target("aarch64-resource-headers" "${aarch64_only_files};${aarch64_only_generated_files}")
-add_header_target("cuda-resource-headers" "${cuda_files};${cuda_wrapper_files}")
+add_header_target("cuda-resource-headers" "${cuda_files};${cuda_wrapper_files};${cuda_wrapper_bits_files}")
 add_header_target("hexagon-resource-headers" "${hexagon_files}")
 add_header_target("hip-resource-headers" "${hip_files}")
 add_header_target("loongarch-resource-headers" "${loongarch_files}")
 add_header_target("mips-resource-headers" "${mips_msa_files}")
 add_header_target("ppc-resource-headers" "${ppc_files};${ppc_wrapper_files}")
 add_header_target("ppc-htm-resource-headers" "${ppc_htm_files}")
 add_header_target("riscv-resource-headers" "${riscv_files};${riscv_generated_files}")
 add_header_target("systemz-resource-headers" "${systemz_files}")
install(
   COMPONENT clang-resource-headers)
 
 install(
   FILES ${cuda_wrapper_files}
   DESTINATION ${header_install_dir}/cuda_wrappers
   COMPONENT clang-resource-headers)
 
 install(
+  FILES ${cuda_wrapper_bits_files}
+  DESTINATION ${header_install_dir}/cuda_wrappers/bits
+  COMPONENT clang-resource-headers)
+
+install(
   FILES ${ppc_wrapper_files}
   DESTINATION ${header_install_dir}/ppc_wrappers
   COMPONENT clang-resource-headers)
 
 install(
   FILES ${openmp_wrapper_files}
   DESTINATION ${header_install_dir}/openmp_wrappers
   COMPONENT clang-resource-headers)
install(
   EXCLUDE_FROM_ALL
   COMPONENT aarch64-resource-headers)
 
 install(
   FILES ${cuda_wrapper_files}
   DESTINATION ${header_install_dir}/cuda_wrappers
   EXCLUDE_FROM_ALL
   COMPONENT cuda-resource-headers)
 
 install(
+  FILES ${cuda_wrapper_bits_files}
+  DESTINATION ${header_install_dir}/cuda_wrappers/bits
+  EXCLUDE_FROM_ALL
+  COMPONENT cuda-resource-headers)
+
+install(
   FILES ${cuda_files}
   DESTINATION ${header_install_dir}
   EXCLUDE_FROM_ALL
   COMPONENT cuda-resource-headers)
 
 install(
   FILES ${hexagon_files}
   DESTINATION ${header_install_dir}