Differential D84334
[flang] Version information in flang/f18 Authored by coti on Jul 22 2020, 9:20 AM.
Details Fixed some version information in flang/f18:
Diff Detail Event TimelineComment Actions 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.
Comment Actions Looks good to me. Please wait for approval from at least one of the other reviewers before merging.
Comment Actions Answered some inline comments.
Comment Actions Replied inline comment from @klausler
Comment Actions Fixed an error in my reply
Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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 :-) Comment Actions @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.
Comment Actions 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:
Comment Actions 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. Comment Actions Here is a new version with, as suggested by @richard.barton.arm:
Also:
Comment Actions Use CMAKE_CURRENT_BINARY_DIR as the target directory of the geneated .h file, as suggested by @isuruf Comment Actions Looking good. Please can you add some tests?
Comment Actions The preprocessing tests are being executed now so that would be a good place to add a test. Comment Actions 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.f90Then #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
^Comment Actions Some tests for the -v flag and associated macros. Some comments:
Comment Actions 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.
Comment Actions Here are the clang definitions: % clang -E -dM -x c /dev/null | grep clang
Comment Actions All the comments should be addressed:
Comment Actions 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. Comment Actions 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! Comment Actions Outch, sorry :( 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. Comment Actions 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"
| ^~~~~~~~~~~~~~~Comment Actions I'm also not sure this is passing? FAIL: Flang :: Preprocessing/compiler_defined_macros.F90 (31682 of 71064)
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: ^ <stdin>:1:1: note: scanning from here ^ Input file: <stdin> -dump-input=help explains the following input dump. Input was: 1: integer, parameter :: flang_major = __flang_major__ check:3'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no 2: integer, parameter :: flang_minor = __flang_minor__ check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3: integer, parameter :: flang_patchlevel = __flang_patchlevel__ check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- Comment Actions @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? Comment Actions 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. Comment Actions Added include ${CMAKE_CURRENT_BINARY_DIR}, as suggested by @isuruf. I tested it on a clean tree, it seems to work.
Comment Actions @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
)Comment Actions 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)
This comment was removed by coti.
Comment Actions 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). Comment Actions Did this work for you? target_include_directories(f18
PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)Comment Actions 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.Comment Actions 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) Comment Actions 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. Comment Actions 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... Comment Actions @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.
Comment Actions 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Can you use CMAKE_CURRENT_BINARY_DIR to configure the file? Source directory should be read only and not written to.