[Libomp] Place generated OpenMP headers into build resource directory #88007
Conversation
jhuber6
requested review from
Meinersbur,
mjklemm,
jdoerfert,
shiltian,
jplehr,
ronlieb,
saiislam and
dhruvachak
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/omp-tools.h DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH} RENAME ompt.h) | ||
| set(LIBOMP_OMP_TOOLS_INCLUDE_DIR ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE) | ||
| install(FILES ${LIBOMP_HEADERS_INTDIR}/omp-tools.h DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH} RENAME ompt.h) | ||
| set(LIBOMP_OMP_TOOLS_INCLUDE_DIR ${LIBOMP_HEADERS_INTDIR} PARENT_SCOPE) |
There was a problem hiding this comment.
FYI this is only required if the build is standalone now. The regular build will always have these included via the resource dir now.
|
If this PR can wait a bit, I can have a look at the Fortran part for this. |
Specifically, clang has In any case, this patch should solve the issues w.r.t. the offload/ move for |
|
Ok. Then let's move the Fortran part to a different PR. |
|
IIRC there is a case where |
| if(${OPENMP_STANDALONE_BUILD}) | ||
| set(LIBOMP_HEADERS_INTDIR ${CMAKE_CURRENT_BINARY_DIR}) | ||
| else() | ||
| set(LIBOMP_HEADERS_INTDIR ${LLVM_BINARY_DIR}/${LIBOMP_HEADERS_INSTALL_PATH}) |
There was a problem hiding this comment.
Is LLVM_BINARY_DIR still valid if user only does -DLLVM_ENABLE_PROJECTS="openmp"? In this case LLVM will not be probably built.
There was a problem hiding this comment.
LLVM is always built, I don't think it's possible to turn it off. Everything goes through cmake ../llvm so it will be set when that CMake runs.
There was a problem hiding this comment.
Is it INSTDIR or INTDIR? What does INTDIR mean?
There was a problem hiding this comment.
Done to match LLVM_RUNTIME_OUTPUT_INTDIR conceptually, It's just intermediate directory. I.e. not where it's being installed, but where it's stored in the build.
Summary: These headers are a part of the compiler's resource directory once installed. However, they are currently placed in the binary directory temporarily. This makes it more difficult to use the compiler out of the build directory and will cause issues when moving to `liboffload`. This patch changes the logic to write these instead to the copmiler's resource directory inside of the build tree. NOTE: This doesn't change the Fortran headers, I don't know enough about those and it won't use the same directory.
|
@llvm/pr-subscribers-clang Author: Joseph Huber (jhuber6) ChangesSummary: NOTE: This doesn't change the Fortran headers, I don't know enough about Full diff: https://github.com/llvm/llvm-project/pull/88007.diff 2 Files Affected:
diff --git a/clang/test/Headers/Inputs/include/stdint.h b/clang/test/Headers/Inputs/include/stdint.h
index 5bf26a7b67b066..67b27b8dfc7b92 100644
--- a/clang/test/Headers/Inputs/include/stdint.h
+++ b/clang/test/Headers/Inputs/include/stdint.h
@@ -16,4 +16,12 @@ typedef unsigned __INTPTR_TYPE__ uintptr_t;
#error Every target should have __INTPTR_TYPE__
#endif
+#ifdef __INTPTR_MAX__
+#define INTPTR_MAX __INTPTR_MAX__
+#endif
+
+#ifdef __UINTPTR_MAX__
+#define UINTPTR_MAX __UINTPTR_MAX__
+#endif
+
#endif /* STDINT_H */
diff --git a/openmp/runtime/src/CMakeLists.txt b/openmp/runtime/src/CMakeLists.txt
index f05bcabb441742..000d02c33dc093 100644
--- a/openmp/runtime/src/CMakeLists.txt
+++ b/openmp/runtime/src/CMakeLists.txt
@@ -10,12 +10,19 @@
include(ExtendPath)
+# The generated headers will be placed in clang's resource directory if present.
+if(${OPENMP_STANDALONE_BUILD})
+ set(LIBOMP_HEADERS_INTDIR ${CMAKE_CURRENT_BINARY_DIR})
+else()
+ set(LIBOMP_HEADERS_INTDIR ${LLVM_BINARY_DIR}/${LIBOMP_HEADERS_INSTALL_PATH})
+endif()
+
# Configure omp.h, kmp_config.h and omp-tools.h if necessary
-configure_file(${LIBOMP_INC_DIR}/omp.h.var omp.h @ONLY)
-configure_file(${LIBOMP_INC_DIR}/ompx.h.var ompx.h @ONLY)
-configure_file(kmp_config.h.cmake kmp_config.h @ONLY)
+configure_file(${LIBOMP_INC_DIR}/omp.h.var ${LIBOMP_HEADERS_INTDIR}/omp.h @ONLY)
+configure_file(${LIBOMP_INC_DIR}/ompx.h.var ${LIBOMP_HEADERS_INTDIR}/ompx.h @ONLY)
+configure_file(kmp_config.h.cmake ${LIBOMP_HEADERS_INTDIR}/kmp_config.h @ONLY)
if(${LIBOMP_OMPT_SUPPORT})
- configure_file(${LIBOMP_INC_DIR}/omp-tools.h.var omp-tools.h @ONLY)
+ configure_file(${LIBOMP_INC_DIR}/omp-tools.h.var ${LIBOMP_HEADERS_INTDIR}/omp-tools.h @ONLY)
endif()
# Generate message catalog files: kmp_i18n_id.inc and kmp_i18n_default.inc
@@ -419,15 +426,15 @@ else()
endif()
install(
FILES
- ${CMAKE_CURRENT_BINARY_DIR}/omp.h
- ${CMAKE_CURRENT_BINARY_DIR}/ompx.h
+ ${LIBOMP_HEADERS_INTDIR}/omp.h
+ ${LIBOMP_HEADERS_INTDIR}/ompx.h
DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH}
)
if(${LIBOMP_OMPT_SUPPORT})
- install(FILES ${CMAKE_CURRENT_BINARY_DIR}/omp-tools.h DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH})
+ install(FILES ${LIBOMP_HEADERS_INTDIR}/omp-tools.h DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH})
# install under legacy name ompt.h
- install(FILES ${CMAKE_CURRENT_BINARY_DIR}/omp-tools.h DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH} RENAME ompt.h)
- set(LIBOMP_OMP_TOOLS_INCLUDE_DIR ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE)
+ install(FILES ${LIBOMP_HEADERS_INTDIR}/omp-tools.h DESTINATION ${LIBOMP_HEADERS_INSTALL_PATH} RENAME ompt.h)
+ set(LIBOMP_OMP_TOOLS_INCLUDE_DIR ${LIBOMP_HEADERS_INTDIR} PARENT_SCOPE)
endif()
if(${BUILD_FORTRAN_MODULES})
set (destination ${LIBOMP_HEADERS_INSTALL_PATH})
|
These are currently installed into clang's directory, this patch just puts it in the resource directory for the build. |
| #ifdef __INTPTR_MAX__ | ||
| #define INTPTR_MAX __INTPTR_MAX__ | ||
| #endif | ||
|
|
||
| #ifdef __UINTPTR_MAX__ | ||
| #define UINTPTR_MAX __UINTPTR_MAX__ | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Are these changes required to make this work?
There was a problem hiding this comment.
Yes, this now installed omp.h in a place where the tests will sometimes find it. In that case we need these extra definitions to make it work.
There was a problem hiding this comment.
Alternatively we could just delete this stdint.h altogether, because this only happens because it's using this one instead. But I assume that we override the regular stdint.h to reduce the number of live definitions in tests or something.
There was a problem hiding this comment.
This header is useful when invoking front end directly such that we don't need to pass things like -internal-isystem which usually are added by compiler driver.
…irectory (llvm#88007)" This reverts commit 8671429. This commit broke the flang build, so I'm reverting it. See the comments in merge request llvm#88007 for more information.
|
Hey, @jhuber. This change broke the flang build. When I revert the change, everything builds cleanly. So I reverted it in merge request #88083. After this change was added, I started getting messages like: |
So the error is caused by your Any suggestions for reproducing it? |
In any case, the |
Oh, I think I get it. We're adding the resource directory include to the include paths. This is not conflicting with the ones from GCC. Honestly we only need this for |
|
I should have mentioned, I'm building with GCC 9.3.0. Most of my team uses this compiler. |
Yeah, that's fine for building LLVM but in the future we'll probably require building the offloading runtime with an up-to-date clang. I think for now I can just restrict this patch to runtimes builds. |
…#88007) Summary: These headers are a part of the compiler's resource directory once installed. However, they are currently placed in the binary directory temporarily. This makes it more difficult to use the compiler out of the build directory and will cause issues when moving to `liboffload`. This patch changes the logic to write these instead to the copmiler's resource directory inside of the build tree. NOTE: This doesn't change the Fortran headers, I don't know enough about those and it won't use the same directory.
…hanges Revert the portion of llvm#75125 which touch the LIBOMP_HEADERS_INSTALL_PATH in standalone build. This change is harmful for standalone builds, since it tries to overwrite the omp.h inside the build compiler. For all-in-one builds, I've confirmed this change is unncessary as llvm#88007 already set it up so that omp.h will be put into the project build clang's resource directory.
…91243) Revert the portion of #75125 which modified the LIBOMP_HEADERS_INSTALL_PATH in standalone build. This change is harmful for real standalone builds (i.e. builds where we build openmp by itself), since it tries to overwrite the `omp.h` inside the build compiler. For all-in-one builds with clang, testing shows this change is unnecessary as #88007 already set up that build configuration so that omp.h will be put into the project build's `clang` resource directory.
Summary:
These headers are a part of the compiler's resource directory once
installed. However, they are currently placed in the binary directory
temporarily. This makes it more difficult to use the compiler out of the
build directory and will cause issues when moving to
liboffload. Thispatch changes the logic to write these instead to the copmiler's
resource directory inside of the build tree.
NOTE: This doesn't change the Fortran headers, I don't know enough about
those and it won't use the same directory.