[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-30 Thread Artem Belevich via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6cdc07a701ee: [CUDA] correctly install 
cuda_wrappers/bits/shared_ptr_base.h (authored by tra).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -432,7 +436,7 @@
 # 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}")
@@ -466,6 +470,11 @@
   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
@@ -508,6 +517,12 @@
   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}


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -432,7 +436,7 @@
 # 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}")
@@ -466,6 +470,11 @@
   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
@@ -508,6 +517,12 @@
   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}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-30 Thread Qiongsi Wu via Phabricator via cfe-commits 
qiongsiwu1 accepted this revision.
qiongsiwu1 added a comment.

In D151503#4381548 , @tra wrote:

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

LGTM! Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-30 Thread Artem Belevich via Phabricator via cfe-commits 
tra added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-30 Thread Artem Belevich via Phabricator via cfe-commits 
tra updated this revision to Diff 526697.
tra added a comment.

Updated according to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -429,7 +433,7 @@
 # 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}")
@@ -463,6 +467,11 @@
   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
@@ -505,6 +514,12 @@
   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}


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -429,7 +433,7 @@
 # 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}")
@@ -463,6 +467,11 @@
   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
@@ -505,6 +514,12 @@
   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}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-29 Thread Qiongsi Wu via Phabricator via cfe-commits 
qiongsiwu1 added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(

tra wrote:
> qiongsiwu1 wrote:
> > qiongsiwu1 wrote:
> > > tra wrote:
> > > > qiongsiwu1 wrote:
> > > > > 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}")
> > > > > ```
> > > > > 
> > > > > 
> > > > 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.
> > > > 
> > > > 
> > > > 
> > > 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`. 
> > Sorry I made a typo in the previous comment. I meant `x86-resource-headers` 
> > when I said `x86_files`. 
> 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.
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

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-26 Thread Artem Belevich via Phabricator via cfe-commits 
tra added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(

qiongsiwu1 wrote:
> qiongsiwu1 wrote:
> > tra wrote:
> > > qiongsiwu1 wrote:
> > > > 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}")
> > > > ```
> > > > 
> > > > 
> > > 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.
> > > 
> > > 
> > > 
> > 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`. 
> Sorry I made a typo in the previous comment. I meant `x86-resource-headers` 
> when I said `x86_files`. 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-26 Thread Artem Belevich via Phabricator via cfe-commits 
tra updated this revision to Diff 526227.
tra added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -405,6 +409,7 @@
  "arm-resource-headers"
  "aarch64-resource-headers"
  "cuda-resource-headers"
+ "cuda-resource-bits-headers"
  "hexagon-resource-headers"
  "hip-resource-headers"
  "hlsl-resource-headers"
@@ -429,7 +434,8 @@
 # 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("cuda-resource-bits-headers" "${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}")
@@ -463,6 +469,11 @@
   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
@@ -505,6 +516,12 @@
   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}
@@ -650,6 +667,9 @@
   add_llvm_install_targets(install-cuda-resource-headers
DEPENDS cuda-resource-headers
COMPONENT cuda-resource-headers)
+  add_llvm_install_targets(install-cuda-resource-bits-headers
+   DEPENDS cuda-resource-bits-headers
+   COMPONENT cuda-resource-headers)
   add_llvm_install_targets(install-hexagon-resource-headers
DEPENDS hexagon-resource-headers
COMPONENT hexagon-resource-headers)


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -405,6 +409,7 @@
  "arm-resource-headers"
  "aarch64-resource-headers"
  "cuda-resource-headers"
+ "cuda-resource-bits-headers"
  "hexagon-resource-headers"
  "hip-resource-headers"
  "hlsl-resource-headers"
@@ -429,7 +434,8 @@
 # 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"

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-26 Thread Qiongsi Wu via Phabricator via cfe-commits 
qiongsiwu1 added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(

qiongsiwu1 wrote:
> tra wrote:
> > qiongsiwu1 wrote:
> > > 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}")
> > > ```
> > > 
> > > 
> > 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.
> > 
> > 
> > 
> 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`. 
Sorry I made a typo in the previous comment. I meant `x86-resource-headers` 
when I said `x86_files`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-26 Thread Qiongsi Wu via Phabricator via cfe-commits 
qiongsiwu1 added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(

tra wrote:
> qiongsiwu1 wrote:
> > 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}")
> > ```
> > 
> > 
> 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.
> 
> 
> 
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`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-26 Thread Artem Belevich via Phabricator via cfe-commits 
tra added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(

qiongsiwu1 wrote:
> 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}")
> ```
> 
> 
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.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-26 Thread Qiongsi Wu via Phabricator via cfe-commits 
qiongsiwu1 added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(

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}")
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

2023-05-25 Thread Artem Belevich via Phabricator via cfe-commits 
tra created this revision.
Herald added subscribers: mattd, carlosgalvezp, bixia, yaxunl.
Herald added a project: All.
tra edited the summary of this revision.
tra edited the summary of this revision.
tra published this revision for review.
tra added reviewers: qiongsiwu1, jlebar.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, jplehr, sstefan1.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151503

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -429,7 +433,7 @@
 # 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}")
@@ -463,6 +467,11 @@
   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


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -267,6 +267,9 @@
   cuda_wrappers/cmath
   cuda_wrappers/complex
   cuda_wrappers/new
+)
+
+set(cuda_wrapper_bits_files
   cuda_wrappers/bits/shared_ptr_base.h
 )
 
@@ -328,7 +331,8 @@
 
 
 # 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 )
 
@@ -429,7 +433,7 @@
 # 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}")
@@ -463,6 +467,11 @@
   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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits