[PATCH] D82659: Fix missing build dependency on omp_gen.

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

Uhm.. it looks like it is not needed anymore. In the `LLVMConfig.cmake` that 
will be installed a `intrinsics_gen` and `omp_gen` custom targets are created 
for exactly the purpose of allowing out-of-tree or standalone builds to freely 
depend on them.
The Clang code where `intrinsics_gen` is conditionally added as a dependency is 
from 2014, while the change in `LLVMConfig.cmake.in` is from 2017.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Valentin Clement via Phabricator via cfe-commits]

clementval added a comment.

In D82659#2138228 , @michele.scandale 
wrote:

> In D82659#2136999 , @clementval 
> wrote:
>
> > Looks good but just one question ... When clang is built as standalone it 
> > does not build the OpenMP part inside Clang? I haven't seen any code to 
> > avoid compiling the OpenMP parsing and semantic checking inside clang.
>
>
> I don't think there is a way to avoid compiling the OpenMP support in Clang. 
> The standalone build is just building the content of the `clang` directory as 
> a separate CMake project reusing the an already built LLVM -- therefore the 
> `libLLVMFrontendOpenMP` as well as the `OMP.h.inc` would have been generated 
> already.


Ok then your fix should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

In D82659#2136999 , @clementval wrote:

> Looks good but just one question ... When clang is built as standalone it 
> does not build the OpenMP part inside Clang? I haven't seen any code to avoid 
> compiling the OpenMP parsing and semantic checking inside clang.


I don't think there is a way to avoid compiling the OpenMP support in Clang. 
The standalone build is just building the content of the `clang` directory as a 
separate CMake project reusing the an already built LLVM -- therefore the 
`libLLVMFrontendOpenMP` as well as the `OMP.h.inc` would have been generated 
already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Valentin Clement via Phabricator via cfe-commits]

clementval added a comment.

In D82659#2136909 , @michele.scandale 
wrote:

> Why `omp_gen` is now a dependency of `clang-tablegen-targets` rather than 
> being in the `LLVM_COMMON_DEPENDS` list like `clang-tablegen-targets`?
>
> Moreover I've noticed that with the recent changes where  `omp_gen` has been 
> added as a dependency in several libraries, this was done unconditionally 
> breaking the Clang standalone build.
>  For the same issue `intrinsics_gen` is added only if `CLANG_BUILT_STANDALONE 
> ` is false.
>
> At this point I think that something like:
>
>   # All targets below may depend on all tablegen'd files.
>   get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
>   add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
>   set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
>   list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
>   if(NOT CLANG_BUILT_STANDALONE)
> list(APPEND LLVM_COMMON_DEPENDS omg_gen)
>   endif()
>
>
> would fix all the issues, and it would allow removing the explicit 
> dependencies added to each clang library.
>
> Is there any issue with my reasoning?


Looks good but just one question ... When clang is built as standalone it does 
not build the OpenMP part inside Clang? I haven't seen any code to avoid 
compiling the OpenMP parsing and semantic checking inside clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

Why `omp_gen` is now a dependency of `clang-tablegen-targets` rather than being 
in the `LLVM_COMMON_DEPENDS` list like `clang-tablegen-targets`?

Moreover I've noticed that with the recent changes where  `omp_gen` has been 
added as a dependency in several libraries, this was done unconditionally 
breaking the Clang standalone build.
For the same issue `intrinsics_gen` is added only if `CLANG_BUILT_STANDALONE ` 
is false.

At this point I think that something like:

  # All targets below may depend on all tablegen'd files.
  get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
  add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
  set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
  list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
  if(NOT CLANG_BUILT_STANDALONE)
list(APPEND LLVM_COMMON_DEPENDS omg_gen)
  endif()

would fix all the issues, and it would allow removing the explicit dependencies 
added to each clang library.

Is there any issue with my reasoning?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Valentin Clement via Phabricator via cfe-commits]

clementval added a comment.

In D82659#2130022 , @simon_tatham 
wrote:

> @jdoerfert , @clementval : over in D83032  
> is a polished-up version of the script I used to check where the missing deps 
> needed to go. Might be useful for the next problem of this kind. But I'm not 
> sure who to get to review it; perhaps one of you might look at it?


No idea who can review this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

@jdoerfert , @clementval : over in D83032  is 
a polished-up version of the script I used to check where the missing deps 
needed to go. Might be useful for the next problem of this kind. But I'm not 
sure who to get to review it; perhaps one of you might look at it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Thank you both for figuring this out!

*Much* appreciated!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Simon Tatham via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e6f19fd8390: Fix missing build dependency on omp_gen. 
(authored by simon_tatham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -517,7 +517,10 @@
 
 # All targets below may depend on all tablegen'd files.
 get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
-add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
+add_custom_target(clang-tablegen-targets
+  DEPENDS
+  omp_gen
+  ${CLANG_TABLEGEN_TARGETS})
 set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
 list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
 


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -517,7 +517,10 @@
 
 # All targets below may depend on all tablegen'd files.
 get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
-add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
+add_custom_target(clang-tablegen-targets
+  DEPENDS
+  omp_gen
+  ${CLANG_TABLEGEN_TARGETS})
 set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
 list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

[facepalm] Thank you. I carefully //wrote// a revised description, but forgot 
to upload it to this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

In D82659#2125618 , @clementval wrote:

> LGTM. So later the DEPENDS omp_gen that are in clang subdirectories could be 
> removed right?


That seems likely. I'm thinking what I should probably do is to polish up my 
checking script and try to get it into `llvm/utils` somewhere, and then it 
would be possible to make that kind of change and be //confident// that it 
wasn't removing a necessary dependency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Valentin Clement via Phabricator via cfe-commits]

clementval added a comment.

@simon_tatham Can you just update the description so it is consistent with your 
fix when it lands?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Valentin Clement via Phabricator via cfe-commits]

clementval accepted this revision.
clementval added a comment.

LGTM. So later the DEPENDS omp_gen that are in clang subdirectories could be 
removed right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659



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

[PATCH] D82659: Fix missing build dependency on omp_gen.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham updated this revision to Diff 274739.
simon_tatham added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Here's a completely different patch, which adds all the missing dependencies on 
`OMP.h.inc` in the `clang` subdirectory in one go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82659

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -517,7 +517,10 @@
 
 # All targets below may depend on all tablegen'd files.
 get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
-add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
+add_custom_target(clang-tablegen-targets
+  DEPENDS
+  omp_gen
+  ${CLANG_TABLEGEN_TARGETS})
 set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
 list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
 


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -517,7 +517,10 @@
 
 # All targets below may depend on all tablegen'd files.
 get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
-add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
+add_custom_target(clang-tablegen-targets
+  DEPENDS
+  omp_gen
+  ${CLANG_TABLEGEN_TARGETS})
 set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
 list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits