Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
4

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
Herald added a subscriber: llvm-commits. ยท View Herald Transcript

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
84โ€“86

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.