[flang] Put ISO_Fortran_binding.h where it can be easily used #68756
Conversation
The update stems from the discussion in https://discourse.llvm.org/t/adding-flang-specific-header-files-to-clang/72442 I decided to put ISO_Fortran_binding.h in a place where it would be accessible with the include: "#include<ISO_Fortran_binding.h>" rather than "#include<fortran/ISO_Fortran_binding.h>" because this is what gfortran does. The changes to put the header file into the build and install areas are in .../flang/CMakeLists.txt. This code calls "get_clang_resource_dir", which requires "PACKAGE_VERSION" to be defined, so I added code to define it. Note that the file is also installed into ".../include/flang", so if a user wanted to access the file from a compiler other than clang, it would be available. I added a test in ".../flang/test/Examples". Since the flang project depends on clang, clang will always be available in a flang build. To make the test work, I also needed to put ISO_Fortran_binding.h into the build area.
psteinfeld
requested review from
tstellar,
banach-space,
kiranchandramohan and
vzakhari
flang/test/Examples/ctofortran.f90
Outdated
| ! UNSUPPORTED: system-windows | ||
| ! RUN: split-file %s %t | ||
| ! RUN: %clang -c %t/cfile.c | ||
| ! RUN: %flang -flang-experimental-hlfir cfile.o %t/src.f90 |
There was a problem hiding this comment.
| ! RUN: %flang -flang-experimental-hlfir cfile.o %t/src.f90 | |
| ! RUN: %flang -flang-experimental-hlfir cfile.o %t/src.f90 -o %t/ctofortran |
flang/test/Examples/ctofortran.f90
Outdated
| ! RUN: split-file %s %t | ||
| ! RUN: %clang -c %t/cfile.c | ||
| ! RUN: %flang -flang-experimental-hlfir cfile.o %t/src.f90 | ||
| ! RUN: a.out | FileCheck %s |
There was a problem hiding this comment.
| ! RUN: a.out | FileCheck %s | |
| ! RUN: %t/ctofortran | FileCheck %s |
There was a problem hiding this comment.
I think it may fail because of the missing "./", but I would suggest a named executable file to make it more explicit.
There was a problem hiding this comment.
Thanks, Slava. You're a prince!
flang/test/CMakeLists.txt
Outdated
| @@ -64,6 +64,7 @@ set(FLANG_TEST_DEPENDS | |||
| FortranRuntime | |||
| Fortran_main | |||
| FortranDecimal | |||
| clang | |||
There was a problem hiding this comment.
Is this pulling in all of clang? Do we really need that? If this is only for testing, can we use REQUIRES: clang in the test to make sure clang is available?
There was a problem hiding this comment.
Sounds good. I'll remove it.
There was a problem hiding this comment.
REQUIRES: clang only affects whether the LIT test is run or not. The clang keyword, in this case, has to be present in config.available_features for the test to run. Since we do not add clang into the available features list, the test will just not run. Even if we did enabled clang as a LIT feature, we still have to guarantee that the clang target (that builds the actual clang executable) is built before the test runs - this can only be done by setting proper dependency in CMake files.
If building the whole clang under check-flang is not acceptable, we need another solution.
There was a problem hiding this comment.
I generally use check-flang to build flang. Also, was not sure whether to add a dependency with the whole of clang for just one test.
Yes, we have to add clang to config.available_features for REQUIRES to work.
For the dependency with clang, Do we really want to check with the clang just built or is it sufficient to check with the clang installed on the system? Also, is it OK to check with the system compiler?
The OpenMP runtime library checks for the presence of flang for running an offload test. Would that be a possible way?
https://reviews.llvm.org/D158546
Another reference might include the way the runtime no dependency on C++ libs is checked.
https://reviews.llvm.org/D104290
There was a problem hiding this comment.
Another option would be to add this to the llvm-test-suite (https://github.com/llvm/llvm-test-suite/tree/main/Fortran/UnitTests) and ensure that one of the buildbots run it.
Also, I would like to clarify that this is not a blocker issue. If the dependency has to be there and that is what is preferred, we can go ahead.
There was a problem hiding this comment.
For the dependency with clang, Do we really want to check with the clang just built or is it sufficient to check with the clang installed on the system? Also, is it OK to check with the system compiler?
Yes, we need to check with the clang that has just been built. The test is checking that clang is able to find the header file, so clang used for the test is the one which has the header file in its "installation" structure.
If we change the LIT test from clang compilation +flang compilation + execution into just clang compilation, then we can move this functionality into clang/lib/Headers completely.
We can have an end-to-end test with clang/flang/executable in llvm-test-suite as you suggested.
I think adding clang dependency is too much overhead.
There was a problem hiding this comment.
Moving the functionality into clang would make clang dependent on flang, which we don't want. I do think we want to keep the file ISO_Fortran_binding.h in the flang project.
There was a problem hiding this comment.
I do not think the "dependency" I am talking about is a problem. If we move the LIT tests into clang source tree, and do not use flang-new in the test, then the only dependency is the ability for clang CMake code to access ISO_Fortran_binding.h, which is already a part of the LLVM source tree. So it should not be a problem.
Anyway, I am okay with keeping it as-is, but without having the clang dependency for check-flang. Moving the LIT test out of the tree should "resolve" this.
There was a problem hiding this comment.
Is there a way to make this test configurable based on a cmake variable? That way, we could keep in in the flang tree and keep it so that it created and executes a program.
There was a problem hiding this comment.
After talking to Slava. This doesn't seem like a viable alternative.
| @@ -0,0 +1,59 @@ | |||
| ! UNSUPPORTED: system-windows | |||
There was a problem hiding this comment.
Can we also add something like REQUIRES: clang
There was a problem hiding this comment.
Thanks, Kiran. Will do.
|
By the way, I timed doing a full build of clang, flang, and check-flang. On my system, this took 8:30. I then did a build of just check-flang. This took 5:50. |
|
After all of this discussion, I'm convinced that the right place to put this test is in the llvm cross project tests (//github.com/llvm/llvm-project/tree/main/cross-project-tests as was suggested earlier by @tschuett. I'll abandon this change. |
|
New pull request for this in #69121 |
The update stems from the discussion in https://discourse.llvm.org/t/adding-flang-specific-header-files-to-clang/72442 This is my second attempt at this. My first attempt was in pull request #68756. I decided to put ISO_Fortran_binding.h in a place where it would be accessible with the include: "#include<ISO_Fortran_binding.h>" rather than "#include<fortran/ISO_Fortran_binding.h>" because this is what gfortran implements. Note that the file is also installed into ".../include/flang", so if a user wanted to access the file from a compiler other than clang, it would be available. I added a test in ".../flang/test/Examples". To make the test work, I also needed to put ISO_Fortran_binding.h into the build area. Although the flang project depends on clang, clang may not always be available in a flang build. For example, when building just the "check-flang" target, the "clang" executable may not be available at the time the new test gets run. To account for this, I made the test's script check for the existence of the "clang" executable. If "clang" is not available, it simply prints "PASS". If it is available, it fully builds and executes the test. On success, this will also print "PASS"
The update stems from the discussion in https://discourse.llvm.org/t/adding-flang-specific-header-files-to-clang/72442 This is my third attempt at this. My second attempt was in pull request #69121. This is my second attempt at this. My first attempt was in pull request #68756. This pull request has three changes from the second one: - I put the test into the Driver directory rather than Examples so that it would get run without require the define LLVM_BUILD_EXAMPLES. - When installing ISO_Fortran_binding.h, I changed the location where it was installed from. - I changed the test so that it would work when flang was built with shared libraries. Here's the information from my previous attempts: I decided to put ISO_Fortran_binding.h in a place where it would be accessible with the include: "#include<ISO_Fortran_binding.h>" rather than "#include<fortran/ISO_Fortran_binding.h>" because this is what gfortran implements. Note that the file is also installed into ".../include/flang", so if a user wanted to access the file from a compiler other than clang, it would be available. I added a test in ".../flang/test/Driver". To make the test work, I also needed to put ISO_Fortran_binding.h into the build area. Although the flang project depends on clang, clang may not always be available in a flang build. For example, when building just the "check-flang" target, the "clang" executable may not be available at the time the new test gets run. To account for this, I made the test's script check for the existence of the "clang" executable. If "clang" is not available, it simply prints "PASS". If it is available, it fully builds and executes the test. On success, this will also print "PASS"
The update stems from the discussion in
https://discourse.llvm.org/t/adding-flang-specific-header-files-to-clang/72442
I decided to put ISO_Fortran_binding.h in a place where it would be accessible with the include: "#include<ISO_Fortran_binding.h>" rather than "#include<fortran/ISO_Fortran_binding.h>" because this is what gfortran does.
The changes to put the header file into the build and install areas are in .../flang/CMakeLists.txt. This code calls "get_clang_resource_dir", which requires "PACKAGE_VERSION" to be defined, so I added code to define it.
Note that the file is also installed into ".../include/flang", so if a user wanted to access the file from a compiler other than clang, it would be available.
I added a test in ".../flang/test/Examples". Since the flang project depends on clang, clang will always be available in a flang build. To make the test work, I also needed to put ISO_Fortran_binding.h into the build area.