[flang][cmake] Make CMake copy "omp_lib.h" into the build directory

[awarzynski]

Summary

Any header or module file in the Flang source directory is of no use to the compiler unless it is copied into the build directory. Indeed, all compiler search paths are relative to the compiler executable (flang-new in our case). Hence, "omp_lib.h" should be copied into the build directory alongside other compiler-provided files that can be "included" (header files) or "used" (module files).

For now, "omp_lib.h" is copied into "<build-dir>/include/flang/OpenMP". We may decide to change this in future. For example, Clang copies a bunch of runtime headers into “<build-dir>/lib/clang/<version-number>”. We could also consider using a similar header from a different sub-project.

Flang's driver search path is updated accordingly. A rule for "installing" the "omp_lib.h" header is yet to be added (we will also need to determine the suitable location for this).

[kiranchandramohan]

-> why is this change in the f18 tools? Does the flang-new driver share this code?
-> Do we have to copy the module file as well?
-> Our long-term plan is to use the omp_lib.h from the llvm-project/openmp directory. Have not checked whether it will work directly.

[awarzynski]

In D122015#3392641, @kiranchandramohan wrote:

-> why is this change in the f18 tools? Does the flang-new driver share this code?

Because the module files from flang/module dir are processed in the f18 tools dir. And omp_lib.h sits alongside other Flang modules files ATM. And I can't think of a better copy location for this header file beyond <build-dir>/include/flang, which is exactly where module files are copied :)

-> Do we have to copy the module file as well?

We do copy them already: https://github.com/llvm/llvm-project/blob/main/flang/tools/f18/CMakeLists.txt#L46

-> Our long-term plan is to use the omp_lib.h from the llvm-project/openmp directory.

This change is needed so that one doesn't need -I <root_dir>/llvm-project/flang/module when compiling OpenMP files (i.e. files with INCLUDE 'omp_lib.h'). Once we are ready to switch to "omp_lib.h" from llvm-project/OpenMP then we should do it. But I feel that this change is orthogonal - if there's "omp_lib'h" in Flang then we should make sure that we use it.

Have not checked whether it will work directly.

Aye, worth trying then!

[kiranchandramohan]

Can you check the CI failure?

[awarzynski]

Copy "omp_lib.h" into <build-dir>/include/flang/OpenMP

Copying "omp_lib.h" alongside intrinsic module files (i.e. into <build-dir>/include/flang) does not play well with how intrinsic and non-intrinsic module files are searched (see https://reviews.llvm.org/D118631 for a bit more context). Putting "omp_lib.h" into a dedicated directory with no modules files in it is cleaner and should solve the issue flagged by the pre-merge CI.

I feel that we should also make sure that the intrinsic modules are installed into a bit less generic directory (perhaps <build-dir>/include/flang/intrinsic-modules ?). Also, the CMake rules for building/copying them should be moved. flang/tools/f18/CMakeLists.txt does not feel like the right place. I'll prepare a seperate patch for that.

[kiranchandramohan]

flang/lib/Frontend/CompilerInvocation.cpp

385

Nit: Can you call getIntrinsicDir and then append /OpenMP to it?

696

I guess this is needed for inclusion of omp_lib.h in user source and the inclusion in omp_lib.f90 to work correctly. Will its order at the end affect the outcome if there are stray omp_lib.h in the user's directories/somewhere?

flang/tools/f18/CMakeLists.txt

62

I guess there is no way to share the path with getOpenMPHeadersDir.

[awarzynski]

flang/lib/Frontend/CompilerInvocation.cpp

385

That would assume that /OpenMP is a sub-directory of the intrinsic modules dir, but I would actually like to change that sometime soon. More specifically, I would like to move the intrinsic modules to <build-dir>/include/flang/intrinsic-modules/ (or something similar).

696

. is the first directory added to searchDirectories: https://github.com/llvm/llvm-project/blob/main/flang/lib/Frontend/CompilerInvocation.cpp#L627. So omp_lib.h in the current directory should be prioritized over the one shipped with the compiler. I guess that it would make sense to add a test for that! On it :)

On a related note, this call should happen in SetDefaultFortranOpts. I'll move it.

flang/tools/f18/CMakeLists.txt

62

I can't think of an easy way, no :) Also, I've not seen anything like that in Clang. I think that it could be a bit too convoluted in practice. We could always re-visit in the future!

On a different note, "${CMAKE_INSTALL_INCLUDEDIR}/flang/OpenMP/ doesn't work for me anymore :/ I've updated to "${CMAKE_BINARY_DIR}/include/flang/OpenMP/" instead. I guess that initially CMAKE_INSTALL_INCLUDEDIRE == CMAKE_BINARY_DIR, but then I must have changed my CMake configuration.

[awarzynski]

Added a test, moved the call to getOpenMPHeadersDir

[kiranchandramohan]

flang/tools/f18/CMakeLists.txt

62

Wouldn't that mean it will not work for installations?

[kiranchandramohan]

LGTM. Since this patch is only dealing with copying to the build directory.
Please update the summary and remove the mention of the installation directory.

[awarzynski]

Thanks @kiranchandramohan , I've updated the summary and also created https://github.com/llvm/llvm-project/issues/54751 for tracking the outstanding work (i.e. installing "omp_lib.h").

[sscalpone]

flang/lib/Frontend/CompilerInvocation.cpp

389

Clang just puts omp.h in its include directory. Is there a reason to choose a different organization with the introduction of the OpenMP subdirectory? Do we expect more OpenMP files to be in that directory?

Clang also puts the OpenMP tools header files (ompt) in the include directory.

% find . -name omp.h
./lib/clang/15.0.0/include/omp.h
% find . -name ompt.h
./lib/clang/15.0.0/include/ompt.h

flang/tools/f18/CMakeLists.txt

61

For the TODO, do you mean to refer to the source file?

62

I'm no expert, but is OWNER_WRITE needed?

[awarzynski]

Thanks for taking a look @sscalpone !

flang/lib/Frontend/CompilerInvocation.cpp

389

Clang just puts omp.h in its include directory. Is there a reason to choose a different organization with the introduction of the OpenMP subdirectory?

I prepared this patch to unblock building SNAP with LLVM Flang with OpenMP enabled (documented here). Or, more specifically, to make this "just" work:

include "omp_lib.h"

That was the the main goal. In the commit message I hinted that we may want to update the location or the source of "omp_lib.h". I agree that we should be consistent with Clang, but that's an implementation detail that we can always fine tune.

Btw, how did you build Clang? I don't get "omp.h" in my build dir.

flang/tools/f18/CMakeLists.txt

61

The source file itself and the this line as well. Neither belongs here.

TBH, I'd rather act on it than update the comment itself. I started with the module files here (that's just for the CMake rules): https://reviews.llvm.org/D122833

62

Probably not, but does it hurt to have it here?

[sscalpone]

I build openmp

-DLLVM_ENABLE_PROJECTS="clang;mlir;openmp;flang"

I believe the omp-lib.h file belongs with the OpenMP implementation & not the flang implementation, as that's the source of the implementation. I did not check if the C/C++ interface omp.h is created as part of clang or openmp (or elsewhere).

flang/tools/f18/CMakeLists.txt

62

Only when someone accidentally writes the file. Is there some reason to let this slip by?

[awarzynski]

In D122015#3435138, @sscalpone wrote:

I believe the omp-lib.h file belongs with the OpenMP implementation & not the flang implementation, as that's the source of the implementation.

Makes sense to me, but I've not worked in this area much. I'll defer to @kiranchandramohan .

flang/tools/f18/CMakeLists.txt

62

FWIW, files in ./lib/clang/15.0.0/include/ have similar file permissions (in fact, even more relaxed).

[kiranchandramohan]

In D122015#3436644, @awarzynski wrote:

In D122015#3435138, @sscalpone wrote:

I believe the omp-lib.h file belongs with the OpenMP implementation & not the flang implementation, as that's the source of the implementation.

Makes sense to me, but I've not worked in this area much. I'll defer to @kiranchandramohan .

Yes we should pick the one from the OpenMP runtime, we did discuss this three years back. :)
https://github.com/flang-compiler/f18/pull/685#discussion_r318173804

I have created a ticket to remind us. https://github.com/llvm/llvm-project/issues/54800

flang/lib/Frontend/CompilerInvocation.cpp

static std::string getIntrinsicDir() {
   // TODO: Find a system independent API
   llvm::SmallString<128> driverPath;
   driverPath.assign(llvm::sys::fs::getMainExecutable(nullptr, nullptr));
   llvm::sys::path::remove_filename(driverPath);
   driverPath.append("/../include/flang/");
   return std::string(driverPath);
 }
 
+// Generate the path to look for OpenMP headers
+static std::string getOpenMPHeadersDir() {
+  llvm::SmallString<128> includePath;
+  includePath.assign(llvm::sys::fs::getMainExecutable(nullptr, nullptr));
+  llvm::sys::path::remove_filename(includePath);
+  includePath.append("/../include/flang/OpenMP/");
+  return std::string(includePath);
+}
+
 /// Parses all preprocessor input arguments and populates the preprocessor
 /// options accordingly.
 ///
 /// \param [in] opts The preprocessor options instance
 /// \param [out] args The list of input arguments
 static void parsePreprocessorArgs(
     Fortran::frontend::PreprocessorOptions &opts, llvm::opt::ArgList &args) {
   // Add macros from the command line.
void CompilerInvocation::CollectMacroDefinitions() {
   }
 }
 
 void CompilerInvocation::SetDefaultFortranOpts() {
   auto &fortranOptions = fortranOpts();
 
   std::vector<std::string> searchDirectories{"."s};
   fortranOptions.searchDirectories = searchDirectories;
+
+  // Add the location of omp_lib.h to the search directories. Currently this is
+  // identical to the modules' directory.
+  fortranOptions.searchDirectories.emplace_back(getOpenMPHeadersDir());
+
   fortranOptions.isFixedForm = false;
 }
 
 // TODO: When expanding this method, consider creating a dedicated API for
 // this. Also at some point we will need to differentiate between different
 // targets and add dedicated predefines for each.
 void CompilerInvocation::SetDefaultPredefinitions() {
   auto &fortranOptions = fortranOpts();
void CompilerInvocation::SetFortranOpts() {
   // Adding search directories specified by -I
   fortranOptions.searchDirectories.insert(
       fortranOptions.searchDirectories.end(),
       preprocessorOptions.searchDirectoriesFromDashI.begin(),
       preprocessorOptions.searchDirectoriesFromDashI.end());
 
   // Add the ordered list of -intrinsic-modules-path
   fortranOptions.searchDirectories.insert(
       fortranOptions.searchDirectories.end(),
       preprocessorOptions.searchDirectoriesFromIntrModPath.begin(),
       preprocessorOptions.searchDirectoriesFromIntrModPath.end());
 
   //  Add the default intrinsic module directory
   fortranOptions.intrinsicModuleDirectories.emplace_back(getIntrinsicDir());
 
   // Add the directory supplied through -J/-module-dir to the list of search
   // directories

flang/test/Driver/include-omp-header.f90

+! Verify that the omp_lib.h header is found and included correctly. This header file should be available at a path:
+!   * relative to the driver, that's
+!   * known the driver.
+! This is taken care of at the CMake and the driver levels. Note that when searching for header files, the directory of the current
+! source file takes precedence over other search paths. Hence adding omp_lib.h in the current directory will make Flang use that
+! header file instead of the one shipped with Flang.
+
+!----------
+! RUN LINES
+!----------
+! This should just work
+! RUN: not rm omp_lib.h
+! RUN: %flang -fsyntax-only -fopenmp %s  2>&1
+
+! Create an empty omp_lib.h header that _does not_ define omp_default_mem_alloc - this should lead to semantic errors
+! RUN: touch omp_lib.h
+! RUN: not %flang -fsyntax-only -fopenmp %s  2>&1 | FileCheck %s
+! RUN: rm omp_lib.h
+
+!--------------------------
+! EXPECTED OUTPUT
+!--------------------------
+! CHECK: error: Must have INTEGER type, but is REAL(4)
+
+!-------
+! INPUT
+!-------
+include "omp_lib.h"
+
+integer :: x, y
+
+!$omp allocate(x, y) allocator(omp_default_mem_alloc)
+
+end

flang/tools/f18/CMakeLists.txt

 if (NOT WIN32)
   add_custom_command(
     OUTPUT ${CMAKE_BINARY_DIR}/bin/flang
     DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/flang
     COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/flang ${CMAKE_BINARY_DIR}/bin)
   add_custom_target(flang ALL DEPENDS ${CMAKE_BINARY_DIR}/bin/flang)
   install(PROGRAMS ${CMAKE_BINARY_DIR}/bin/flang DESTINATION "${CMAKE_INSTALL_BINDIR}")
 endif()
+
+# TODO Move this to a more suitable location
+file(COPY ${FLANG_SOURCE_DIR}/module/omp_lib.h DESTINATION "${CMAKE_BINARY_DIR}/include/flang/OpenMP/" FILE_PERMISSIONS OWNER_READ OWNER_WRITE)