This is an archive of the discontinued LLVM Phabricator instance.

[flang] Version information in flang/f18
ClosedPublic

Authored by coti on Jul 22 2020, 9:20 AM.

Details

Summary

Fixed some version information in flang/f18:

  • added FLANG, FLANG_MAJOR, FLANG_MINOR__ and FLANG_PATCHLEVEL (similar to their __F18* counterparts) for compatibility purpose

Diff Detail

Event Timeline

coti created this revision.Jul 22 2020, 9:20 AM
coti updated this revision to Diff 279850.Jul 22 2020, 9:33 AM
AlexisPerry requested changes to this revision.Jul 22 2020, 10:07 AM

Thanks for the patch! I had a couple questions and one small change request (see inline comments). The questions truly should be treated as questions rather than requests for changes, since I'm not 100% sure of the right answer to them.

flang/tools/f18/f18.cpp
390

Should we use "flang" instead of "f18" to better align with the project's name?

414–417

Should these replace the __F18 equivalents rather than add on? Also, should they match the clang/llvm version numbers, which I believe are 12.0.0 now?

634

Please change to arg == "--version" to align with the clang, gfortran, and pgf90 flag.

This revision now requires changes to proceed.Jul 22 2020, 10:07 AM
coti updated this revision to Diff 279882.Jul 22 2020, 10:44 AM
coti marked 3 inline comments as done.

Applied suggestions from Alexis.

AlexisPerry accepted this revision.Jul 22 2020, 12:51 PM
AlexisPerry added a subscriber: flang-commits.

Looks good to me. Please wait for approval from at least one of the other reviewers before merging.

This revision is now accepted and ready to land.Jul 22 2020, 12:51 PM
sscalpone accepted this revision.Jul 22 2020, 12:59 PM
sscalpone added inline comments.
flang/tools/f18/f18.cpp
410

Cmake has the capability to generate files, usually taking a "foo.in" and creating a "foo" with macro substitution. Is that technique possible for the version numbers?

klausler added inline comments.
flang/tools/f18/f18.cpp
414

The older flang compiler reportedly defines __FLANG. If so, it might be a bad idea to also define __FLANG in this newer flang compiler,; how would conditionally compiled code distinguish one flang compiler from the other?

coti marked 2 inline comments as done and an inline comment as not done.Jul 22 2020, 2:39 PM

Answered some inline comments.

flang/tools/f18/f18.cpp
390

Good question. I left the original message, I let senior developers make a decision here.

410

Yes it is! Should I "update diff" to show it?

414

So I have a question here: what is the rule regarding compatibility between old and new flang? I came across situations where I needed these because I tried to compile software that knew the old flang and used these values.

klausler added inline comments.Jul 22 2020, 2:50 PM
flang/tools/f18/f18.cpp
414

Does that software that you mentioned use #ifdef __FLANG or #if __FLANG, as opposed to checking the value of the macro?

coti marked an inline comment as done.Jul 22 2020, 3:25 PM

Replied inline comment from @klausler

flang/tools/f18/f18.cpp
414

This is in SuperLU.

It looks for all of them but __FLANG only needs to be defined:

#elif defined(__FLANG)
        PRINT *, 'INFO:compiler[Flang]'
# define COMPILER_VERSION_MAJOR DEC(__FLANG_MAJOR__)
# define COMPILER_VERSION_MINOR DEC(__FLANG_MINOR__)
# if defined(__FLANG_PATCHLEVEL__)
#  define COMPILER_VERSION_PATCH DEC(__FLANG_PATCHLEVEL__)
# endif
coti marked an inline comment as done.Jul 22 2020, 3:29 PM

Fixed an error in my reply

flang/tools/f18/f18.cpp
414
coti updated this revision to Diff 280303.Jul 23 2020, 5:25 PM
coti edited the summary of this revision. (Show Details)

Let cmake grab the version numbers.

I am assuming that f18 will have a MAJOR version number matching the LLVM release? Does classic flang do that as well? Both compilers will probably co-exist for a while and it would be desirable to make them distinguishable to build systems as CMake.

I don't have a solution, but maybe we need to #define __CLASSIC_FLANG in classic flang. I am just throwing out ideas.

I am assuming that f18 will have a MAJOR version number matching the LLVM release? Does classic flang do that as well? Both compilers will probably co-exist for a while and it would be desirable to make them distinguishable to build systems as CMake.

I don't have a solution, but maybe we need to #define __CLASSIC_FLANG in classic flang. I am just throwing out ideas.

I agree with this idea, and if we went this route we could get rid of the duplicate code between __F18_MAJOR and __FLANG_MAJOR (and the others) included in this patch. We would have to coordinate with the Classic Flang developers to make it happen, though, and I'm not sure if they are aware of the discussion happening here.

gak added a subscriber: gak.Jul 27 2020, 9:42 AM

I am assuming that f18 will have a MAJOR version number matching the LLVM release? Does classic flang do that as well? Both compilers will probably co-exist for a while and it would be desirable to make them distinguishable to build systems as CMake.

I don't have a solution, but maybe we need to #define __CLASSIC_FLANG in classic flang. I am just throwing out ideas.

I agree with this idea, and if we went this route we could get rid of the duplicate code between __F18_MAJOR and __FLANG_MAJOR (and the others) included in this patch. We would have to coordinate with the Classic Flang developers to make it happen, though, and I'm not sure if they are aware of the discussion happening here.

At least one is. I think the idea of defining __CLASSIC_FLANG in the compiler is a good one.

Classic Flang's version numbering started out unrelated to the LLVM version numbering. I am not sure how we should change it to align yet, but it's something we can look into.

Although LLVM flang is the spiritual successor to classic flang, I don't think we are specifically aiming for any sort of compatibility between the two compilers, for example on option names, feature support, language support, processor defined behavior, 'bug compatibility', etc. so in this sense the two are completely different compilers. I think it unlikely that a user of classic flang that has conditionalized some code on __FLANG due to some quirk of classic flang or some bug in classic flang that they are working around will necessarily need or want to run the same code for LLVM flang for the same reason. I think this would steer us strongly away from supporting __FLANG in LLVM flang to indicate that it and classic flang are connected in this way.

Could we simply follow what clang does here and use lowercase for the macro name, i.e. __flang_major__ ? That would make LLVM flang totally distinct from classic flang whilst also preserving the obvious meanings of the macros.

I would also suggest matching clang a bit more closely, so using the trailing underscores across the piece and also making __flang__ defined to 1 and adding __flang_version__ to hold the full version number.

I think keeping __F18 for backwards compatibility is fine, but given that the community today is very small (just the developers) if we can take this opportunity to delete it now before more users can come to depend on it then we should do so. I guess the only likely existing users might be @AlexisPerry or someone in @sscalpone 's team?

My proposal would be to support

#define __flang__            1 
#define __flang_major__      "@LLVM_VERSION_MAJOR@"
#define __flang_minor__      "@LLVM_VERSION_MINOR@"
#define __flang_patchlevel__ "@LLVM_VERSION_PATCH@"
#define __flang_version__    "@LLVM_VERSION_MAJOR@.@LLVM_VERSION_MINOR@.@LLVM_VERSION_PATCH@"

+ the existing __F18... flags for backwards compatibility if we absolutely need to.

I also think we should ask to cherry pick this to the LLVM 10 release branch.

Thanks for the patch @coti , we should have thought of this sooner :-)

@richard.barton.arm, I don't think I have any code that relies on __F18 being defined, so I'm alright with removing it and the related macros. You also make a very good point about following clang behavior. I wasn't aware of clang's use of lowercase macros and just assumed everyone uses uppercase macros based on my previous experiences with other codes. I think your proposal is likely the better way to go, first because matching style across LLVM projects is generally a good thing and second because your proposal does not require changes to classic Flang so we don't have to impose on those developers.

Also, +1 on cherry picking this patch to the LLVM 11 release branch once it is merged.

flang/tools/f18/f18.cpp
392

If we adopt @richard.barton.arm's proposal, I think it would be a good idea to make use of the __flang_version__ macro here so that the output actually prints the version number.

coti added a comment.Jul 29 2020, 4:01 PM

I have made the modifications suggested by @richard.barton.arm, but then, CMake does not compile anymore because it relies on __FLANG, __FLANG_MAJOR__, __FLANG_MINOR__ and __FLANG_PATCHLEVEL__.

I have two options there:

I don't think it is safe to assume that LLVM Flang will always be the same as classic Flang for all of the same behaviors that CMake cares about. I think we should be considering LLVM Flang as a new compiler rather than as a different version of classic Flang. I think that means we need to make a change in CMake to add support for LLVM Flang as a new compiler.

coti updated this revision to Diff 281942.Jul 30 2020, 8:34 AM

Here is a new version with, as suggested by @richard.barton.arm:

  • removed __F18*
  • added __flang*

Also:

  • The version numbers are obtained from the project by CMake
  • As suggested by @AlexisPerry, flang -v displays the version number
isuruf added a subscriber: isuruf.Jul 30 2020, 10:13 AM
isuruf added inline comments.
flang/tools/f18/CMakeLists.txt
64

Can you use CMAKE_CURRENT_BINARY_DIR to configure the file? Source directory should be read only and not written to.

coti updated this revision to Diff 282005.Jul 30 2020, 11:38 AM

Use CMAKE_CURRENT_BINARY_DIR as the target directory of the geneated .h file, as suggested by @isuruf

richard.barton.arm requested changes to this revision.Aug 4 2020, 4:41 AM

Looking good. Please can you add some tests?

  • For --version, there already is one for the version screen in flang/test/Driver that could be updated to capture the version number also and run multiple times to test the synonyms.
    • Testing {{\-v}} when it means verbose is hard so suggest not doing that. We need {{-###}} really to test the driver more, which is outside the scope of your change.
  • For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.
This revision now requires changes to proceed.Aug 4 2020, 4:41 AM
tskeith added a subscriber: tskeith.Aug 4 2020, 7:40 AM
  • For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.

The preprocessing tests are being executed now so that would be a good place to add a test.

Looking good. Please can you add some tests?

  • For --version, there already is one for the version screen in flang/test/Driver that could be updated to capture the version number also and run multiple times to test the synonyms.
    • Testing {{\-v}} when it means verbose is hard so suggest not doing that. We need {{-###}} really to test the driver more, which is outside the scope of your change.
  • For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.

Please don't delete the tests in flang/test/Preprocessing.

The preprocessing tests are being executed now so that would be a good place to add a test.

Ha - if only I had written that comment yesterday it would have been right :-) Thanks for enabling them @klausler and @tskeith for the correction.

coti marked an inline comment as done.Aug 4 2020, 3:06 PM

I am writing the tests.

I have a little problem with` flang_version. It looks like f18 considers it as a numeric constants that it cannot interpret (probably because it contains too many dots). For example:

print *, "VERSION", __flang_version__

Gives me:

/home/users/coti/toto.f90:10:4: error: expected execution part construct
    print *, "VERSION", __flang_version__
     ^
/home/users/coti/toto.f90:10:3: in the context: execution part
    print *, "VERSION", __flang_version__
    ^
/home/users/coti/toto.f90:1:1: in the context: main program
  program version
  ^
f18: could not parse /home/users/coti/toto.f90

Then

#if __flang_version__ == "12.0.0"
  print *, "Correct version number"
#endif

gives me:

/home/users/coti/toto.f90:15:5: error: Uninterpretable numeric constant '12.0.0'
  #if __flang_version__ == "12.0.0"
      ^^^^^^^^^^^^^^^^^
in a macro defined here
that expanded to:
  12.0.0
  ^
coti updated this revision to Diff 283086.EditedAug 4 2020, 6:13 PM

Some tests for the -v flag and associated macros.

Some comments:

  • I could not find a way to exploit __flang_version__, since it is interpreted as a numeric constant instead of a string (see above)
  • The version number is simply matched against a regex. I can retrieve it from cmake, similarly to what is done in f18.cpp, but I chose to keep things simple and not add anything into flang/test/CMakeLists.txt. What do you think?

Thanks for adding the tests. IMO, the regex approach in the test is just fine.

I have made a couple of small suggestions and am happy to approve on the basis that those are addressed.

I think one of @AlexisPerry or @sscalpone should also approve before you push.

Thanks for the work on this.

flang/test/Preprocessing/macros.F90
1 ↗(On Diff #283086)

Suggest this be renamed "compiler_defined_macros" or "predefined_macros" to separate it from a general test on user-defined macros.

flang/tools/f18/f18.cpp
418

Given the bug that you have found with flang_version, I suggest we don't define the external macro in this patch, given that it won't be usable and it's essentially exposing the bug that wasn't there before.

This revision is now accepted and ready to land.Aug 5 2020, 12:16 AM
tskeith retitled this revision from Version information in flang/f18 to [flang] Version information in flang/f18.Aug 5 2020, 7:00 AM
tskeith requested changes to this revision.Aug 5 2020, 7:23 AM
tskeith added inline comments.
flang/test/Preprocessing/macros.F90
7 ↗(On Diff #283086)

Does this depend on having gfortran available? It should use %f18 -E like the others.

This revision now requires changes to proceed.Aug 5 2020, 7:23 AM

Here are the clang definitions:

% clang -E -dM -x c /dev/null | grep clang
#define clang 1
#define clang_major 12
#define clang_minor 0
#define clang_patchlevel 0
#define clang_version "12.0.0 (ssh://git@gitlab-master.nvidia.com:12051/fortran/fir-dev-mirror.git 36a44dbf0b4e55917b70fbf209d8c992249e651b)"

flang/tools/f18/f18.cpp
392

Perhaps the message could be "flang front end" ?

coti updated this revision to Diff 283407.Aug 5 2020, 3:06 PM
coti marked 3 inline comments as done.

All the comments should be addressed:

  • renamed the test for pre-processor macros compiler_defined_macros.F90
  • removed __flang_version__ (in f18.cpp and in the tests)
  • use %f18 -E in the pre-processor test in order to avoid a full compilation
tskeith added inline comments.Aug 5 2020, 3:35 PM
flang/test/Preprocessing/compiler_defined_macros.F90
3 ↗(On Diff #283407)

I don't know if you can count on grep being available everywhere. I think FileCheck is better anyway. E.g. something like this:

!CHECK: flang_major = {{[0-9]+$}}
integer, parameter :: flang_major = __FLANG_MAJOR__
coti updated this revision to Diff 283438.Aug 5 2020, 4:22 PM

Thank you @tskeith for the tip on the tests!

tskeith accepted this revision.Aug 5 2020, 4:41 PM

Thanks, looks good to me.

This revision is now accepted and ready to land.Aug 5 2020, 4:41 PM
AlexisPerry accepted this revision.Aug 6 2020, 12:10 PM

Do you have commit access @coti ?

coti added a comment.Aug 7 2020, 9:23 AM

No, I don't ;)

It needs rebasing - please can you do this then I will commit it on your behalf?

coti updated this revision to Diff 283939.Aug 7 2020, 10:08 AM

Updated my local clone, fixed the conflicts and re-generated the diff.

I'm afraid that still doesn't work. My master is on 7d0f691.

coti updated this revision to Diff 284004.Aug 7 2020, 12:55 PM

I generated the diff again on a clean clone. I don't know why but my CMakeList.txt was wrong even after a git stash. Sorry about that, I hope it should be fine now.

I'm afraid arcanist does not apply the patch well. It does not like the new file creation. I'm not sure how you are creating this diff, but are you able to re-upload a diff created with diff -Naur to handle the new files correctly?

$ arc patch D84334 --nobranch
 INFO  Base commit is not in local repository; trying to fetch.
Checking patch flang/tools/f18/f18_version.h.in...
error: flang/tools/f18/f18_version.h.in: does not exist in index
Checking patch flang/tools/f18/f18.cpp...
Checking patch flang/tools/f18/CMakeLists.txt...
Checking patch flang/test/Preprocessing/compiler_defined_macros.F90...
error: flang/test/Preprocessing/compiler_defined_macros.F90: does not exist in index
Checking patch flang/test/Driver/version_test.f90...
Applied patch flang/tools/f18/f18.cpp cleanly.
Applied patch flang/tools/f18/CMakeLists.txt cleanly.
Applied patch flang/test/Driver/version_test.f90 cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!
coti updated this revision to Diff 284333.Aug 10 2020, 6:00 AM

Outch, sorry :(
I create my patches with git diff....

Thank you for showing me the arc command, this time I checked:

$ arc patch --patch ~/f18_version.patch 
 INFO  Base commit is not in local repository; trying to fetch.
Branch name arcpatch already exists; trying a new name.
Branch name arcpatch_1 already exists; trying a new name.
Branch name arcpatch_2 already exists; trying a new name.
Created and checked out branch arcpatch_3.
Checking patch flang/test/Driver/version_test.f90...
Checking patch flang/test/Preprocessing/compiler_defined_macros.F90...
Checking patch flang/tools/f18/CMakeLists.txt...
Checking patch flang/tools/f18/f18.cpp...
Checking patch flang/tools/f18/f18_version.h.in...
Applied patch flang/test/Driver/version_test.f90 cleanly.
Applied patch flang/test/Preprocessing/compiler_defined_macros.F90 cleanly.
Applied patch flang/tools/f18/CMakeLists.txt cleanly.
Applied patch flang/tools/f18/f18.cpp cleanly.
Applied patch flang/tools/f18/f18_version.h.in cleanly.
Usage Exception: User aborted the workflow.

Please can you send me your email address offline for the commit log?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2020, 8:18 AM

This commit has broken the out of tree builds.

llvm-project/flang/tools/f18/f18.cpp:41:10: fatal error: f18_version.h: No such file or directory
   41 | #include "f18_version.h"
      |          ^~~~~~~~~~~~~~~

This commit has broken the out of tree builds.

This probably needs to include CMAKE_CURRENT_BINARY_DIR as a include directory.

I'm also not sure this is passing?


FAIL: Flang :: Preprocessing/compiler_defined_macros.F90 (31682 of 71064)

  • TEST 'Flang ::

Preprocessing/compiler_defined_macros.F90' FAILED ****

Script:

...

Exit Code: 1

Command Output (stderr):

/usr/local/google/home/echristo/sources/llvm-project/flang/test/Preprocessing/compiler_defined_macros.F90:3:9:
error: CHECK: expected string not found in input
!CHECK: flang_major = {{[1-9][0-9]*$}}

^

<stdin>:1:1: note: scanning from here
integer, parameter :: flang_major = flang_major
^
<stdin>:1:23: note: possible intended match here
integer, parameter :: flang_major = flang_major

^

Input file: <stdin>
Check file:
/usr/local/google/home/echristo/sources/llvm-project/flang/test/Preprocessing/compiler_defined_macros.F90

-dump-input=help explains the following input dump.

Input was:
<<<<<<

1: integer, parameter :: flang_major = __flang_major__

check:3'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no
match found
check:3'1 ? possible
intended match

2: integer, parameter :: flang_minor = __flang_minor__

check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

3: integer, parameter :: flang_patchlevel = __flang_patchlevel__

check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

--

I have reverted the patch

@coti - I've had another look and I think your new test is stumbling across an existing bug. If you play around with the existing __F18* macros, you can see that they are not being applied correctly in the code:

ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ cat test.f90
PROGRAM FOO
  integer i = __F18__
end program foo
ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ ../../build/bin/flang -DFOO=bar test.f90 -E
program foo
integer i = __f18__
end program foo

So I don't think there is anything wrong with the code changes or test, but perhaps another bug to fix in the preprocessor itself. Its the end of the day for me now, but I'll look again tomorrow.

I have not looked at the out of tree build issues. Have you tried isuru's suggestion?

@coti - I've had another look and I think your new test is stumbling across an existing bug. If you play around with the existing __F18* macros, you can see that they are not being applied correctly in the code:

ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ cat test.f90
PROGRAM FOO
  integer i = __F18__
end program foo
ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ ../../build/bin/flang -DFOO=bar test.f90 -E
program foo
integer i = __f18__
end program foo

So I don't think there is anything wrong with the code changes or test, but perhaps another bug to fix in the preprocessor itself. Its the end of the day for me now, but I'll look again tomorrow.

I have not looked at the out of tree build issues. Have you tried isuru's suggestion?

I think I remember fixing something in that area with predefined (command-line -D flag) options; I'll see whether it still needs to be pushed.

coti updated this revision to Diff 285145.Aug 12 2020, 11:19 AM

Added include ${CMAKE_CURRENT_BINARY_DIR}, as suggested by @isuruf. I tested it on a clean tree, it seems to work.

coti updated this revision to Diff 285146.Aug 12 2020, 11:20 AM

Typo in a comment.

DavidTruby added inline comments.Aug 12 2020, 5:09 PM
flang/tools/f18/CMakeLists.txt
33

as a small nit: I think target_include_directories(f18 ${CMAKE_CURRENT_BINARY_DIR}) is slightly cleaner. I'm not that fussed about it though

coti added a comment.Aug 12 2020, 5:11 PM

@DavidTruby so, how about replacing:

set(include ${FLANG_BINARY_DIR}/include/flang)

set(include ${FLANG_BINARY_DIR}/include/flang)
set(include ${CMAKE_CURRENT_BINARY_DIR})

with:

target_include_directories(f18
  ${CMAKE_CURRENT_BINARY_DIR}
  ${FLANG_BINARY_DIR}/include/flang 
)

@DavidTruby so, how about replacing:

set(include ${FLANG_BINARY_DIR}/include/flang)

set(include ${FLANG_BINARY_DIR}/include/flang)
set(include ${CMAKE_CURRENT_BINARY_DIR})

with:

target_include_directories(f18
  ${CMAKE_CURRENT_BINARY_DIR}
  ${FLANG_BINARY_DIR}/include/flang 
)

looks good to me!

This worked for me:

target_include_directories(f18
  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)

And I got rid of the duplicate:

set(include ${FLANG_BINARY_DIR}/include/flang)
flang/tools/f18/CMakeLists.txt
82–83

Perhaps using FLANG_BINARY_DIR will fix the build?

coti updated this revision to Diff 285226.Aug 13 2020, 12:17 PM
This comment was removed by coti.
coti updated this revision to Diff 285461.Aug 13 2020, 12:19 PM

Followed @DavidTruby's recommendation on how include directories are set by CMake.

sscalpone reopened this revision.Aug 13 2020, 1:24 PM
sscalpone added inline comments.
flang/tools/f18/CMakeLists.txt
30

Did this work for you?

CMake requires a PUBLIC or PRIVATE qualifier before the first directory argument.

I'm getting:

CMake Error at /local/home/sjs/llvm/llvm-project/flang/tools/f18/CMakeLists.txt:34 (target_include_directories):
  target_include_directories called with invalid arguments
This revision is now accepted and ready to land.Aug 13 2020, 1:24 PM
coti added a comment.Aug 13 2020, 6:57 PM

Actually, I cannot find a way to make it work with target_include_directories on a clean build tree.

With PRIVATE I get an error at the next step, what is being compiled is expecting an input on stdin:

[100%] Built target f18
[100%] Generating /__fortran_builtins.mod
Enter Fortran source
Use EOF character (^D) to end file

With PUBLIC

CMake Error in /home/users/coti/tmp/llvm-project/flang/tools/f18/CMakeLists.txt:
  Target "f18" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/home/users/coti/tmp/tmpbuild/tools/flang/include/flang"

  which is prefixed in the build directory.


CMake Error in /home/users/coti/tmp/llvm-project/flang/tools/f18/CMakeLists.txt:
  Target "f18" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/home/users/coti/tmp/tmpbuild/tools/flang/tools/f18"

  which is prefixed in the build directory.

But, why not put f18_version.h in ${FLANG_BINARY_DIR}/include/flang? This way we only need to include this directory, and it is not totally irrelevant to have the version numbers in this directory, is it? However, I still cannot make target_include_directories work (same as the first error).

sscalpone requested changes to this revision.Aug 13 2020, 10:23 PM

Did this work for you?

target_include_directories(f18
  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)
This revision now requires changes to proceed.Aug 13 2020, 10:23 PM
coti added a comment.Aug 14 2020, 12:15 PM

On a clean build, with:

target_include_directories(f18
    PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)

I get:

[ 91%] Building CXX object tools/flang/tools/f18/CMakeFiles/f18.dir/f18.cpp.o
/home/users/coti/tmp/llvm-project/flang/tools/f18/f18.cpp:40:10: fatal error: f18_version.h: No such file or directory
 #include "f18_version.h"
          ^~~~~~~~~~~~~~~
compilation terminated.

Sorry, I'm not sure what to do next. I'm using cmake vesion 3.18.0 and gcc 9.3.0.

Here's the full diff of my CMakefile:

diff --git a/flang/tools/f18/CMakeLists.txt b/flang/tools/f18/CMakeLists.txt
index 6103117123e..d7987e6341e 100644
--- a/flang/tools/f18/CMakeLists.txt
+++ b/flang/tools/f18/CMakeLists.txt
@@ -9,0 +10,3 @@ add_flang_tool(f18
+target_include_directories(f18
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
+)
@@ -32,2 +34,0 @@ set(include ${FLANG_BINARY_DIR}/include/flang)
-set(include ${FLANG_BINARY_DIR}/include/flang)
-
@@ -82,0 +84 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/flang.sh.in ${FLANG_BINARY_DIR}/bin/f
+configure_file(${CMAKE_CURRENT_SOURCE_DIR}/f18_version.h.in ${CMAKE_CURRENT_BINARY_DIR}/f18_version.h @ONLY)

Steve's patch works for me (cmake 3.17). @coti , your error message looks like what would happen if you replaced both definitions of {{include}} with the {{target_include_directories}} call. {{include}} is set up to the location of pre-built modules and is used in the compilation of said modules and then pre-processed into the flang.sh script. You still need that as well as setting {{target_include_directories}} to tell CMake where f18_version.h is when building f18.cpp.

Note for future (not part of this patch) those modules ought to end up in the lib directory of the install not the include directory. Clang puts similar headers in lib/clang/<version>/include and we should ultimately do the same for Flang. I think the include dir in the installation is for LLVM header files, e.g. if you wanted to use the LLVM libraries in some project or other.

coti updated this revision to Diff 287336.Aug 24 2020, 3:51 AM

On a clean tree, with the attached patch, I get:

[ 90%] Built target f18
[ 90%] Generating /__fortran_builtins.mod
Enter Fortran source
Use EOF character (^D) to end file

And then it does not progress anymore...

@coti the reason for the build failure is because you have removed both of the duplicate definitions of include in flang/tools/f18/CMakeLists.txt. This causes CMake not to be able to find the right files when it creates the builtin module files. One of these needs to be restored in your patch for the build to work as before.

I have marked the line in the original source that needs to be reinstated to your patch to make it work correctly for my in-tree build. If you re-submit a patch with that change then it can be tested out of tree.

flang/tools/f18/CMakeLists.txt
32

This is the line to reinstate.

coti updated this revision to Diff 287416.Aug 24 2020, 9:20 AM

Fixed the include paths. Thank you @richard.barton.arm!

This is working well for me in an in-tree build.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 1 2020, 11:06 AM
This revision was automatically updated to reflect the committed changes.

I have tested this works well on an out of tree build (as it broke it before) so have no reservations about pushing the change again on @coti 's behalf.

This comment was removed by AlexisPerry.
This comment was removed by naromero77.