Page MenuHomePhabricator

[test-suite] Support Fortran tests in CMake infrastructure.
ClosedPublic

Authored by naromero77 on Dec 23 2020, 1:34 PM.

Details

Summary

In preparation for Flang compiler, add support for Fortran in CMake. Include a simple Fortran90 hello world test. Tested with GFortran.

Event Timeline

naromero77 created this revision.Dec 23 2020, 1:34 PM
naromero77 requested review of this revision.Dec 23 2020, 1:34 PM
jdoerfert added inline comments.Jan 5 2021, 8:34 AM
CMakeLists.txt
32

Aren't CMAKE variables by convention upper case only?

Meinersbur added inline comments.Jan 5 2021, 8:36 AM
CMakeLists.txt
9–13

Did you consider using enable_language? It supports an OPTIONAL flag for autodetect the presence of a Fortran compiler. (Not suggesting to use it, it might break buildbots that happen to have gfortran installed, and you will probably not want to run tests for a well-tested distribution provided compiler).

30

[Style] In other places in this file there is no space between if and (

31

There is probably a reason why the _COMPILER vars are marked as advanced, they cannot be changed after the initial configuration. I don't see a reason to make it non-advanced.

42

[not a change requirest] While good here for consistency with C/CXX, newer Cmake should use add_compile_definitions since _FLAGS is supposed to be controlled by the user, and TEST_SUITE_EXTRA_Fortran_FLAGS not be necessary.

197

[nit] unrelated whitespace change

SingleSource/UnitTests/Fortran/CMakeLists.txt
2

Isn't this redundant with what llvm_singlesource does anyway?

Meinersbur added inline comments.Jan 5 2021, 9:41 AM
CMakeLists.txt
32

Weirdly, cmake defines these as CMAKE_<Lang>_COMPILER, where <Lang> is Fortran, in Pascal case. Also see https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER.html and explcitly shown at https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html.

naromero77 added inline comments.Jan 6 2021, 10:38 AM
CMakeLists.txt
9–13

Yes, I looked into that, but the OPTIONAL flag does not work at the moment according to the CMake docs.

https://cmake.org/cmake/help/latest/command/enable_language.html

30

I will fix this.

31

Was this just a comment or where there any action you wanted me to to take?

Here I am just following a parallel constructions similar to the prior mark_as_advance(...).

32

As @Meinersbur notes, for some reason CMake uses Fortran instead of FORTRAN.

42

Thanks for the comments. I noted some other older CMake-isms but did not want to change to much in the first pass.

197

I will fix this.

SingleSource/UnitTests/Fortran/CMakeLists.txt
2

Good question and I didn't know the answer until after I did a little bit of experimentation.

Source is an input variable into llvm_singlesource(), without it the target is built in the wrong directory and CMake complains.

The structure of CMakeLists.txt is similar to what is done elsewhere in the testsuite:
https://github.com/llvm/llvm-test-suite/blob/master/SingleSource/UnitTests/C%2B%2B11/CMakeLists.txt#L4-L9

naromero77 added inline comments.Jan 6 2021, 10:39 AM
CMakeLists.txt
9–13

I will make this change as well.

Meinersbur added inline comments.Jan 6 2021, 11:47 AM
CMakeLists.txt
31

OK, didn't notice this is also done for CMAKE_C_COMPILER, CMAKE_CXX_COMPILER. I don't this is good, but keeps the Fortran support consistent with C/CXX, like modifying _FLAGS.

SingleSource/UnitTests/Fortran/CMakeLists.txt
2

There's something wrong if this is not working without defining Source. See cmake/modules/SingleMultiSource.cmake:

if(DEFINED Source)
  set(sources ${Source})
else()
  file(GLOB sources *.c *.cpp *.cc *.f *.F *.f90 *.F90)
endif()

i.e. if Source is undefined, it does the file search by itself. Defining Source should only be necessary if there are additional source files that should not be compiled. I don't know why SingleSource/UnitTests/C++11 does it this way. The majority of other programs rely on that it does work.

  • Use CMake enable_language instead.
  • Fix style on conditionals.
  • Cleaner implementation of Fortran unit test example.
naromero77 added inline comments.Jan 6 2021, 12:36 PM
SingleSource/UnitTests/Fortran/CMakeLists.txt
2

Thanks for pushing on this.

The problem was that Fortran tests came at the end of the CMakeLists.txt file, so unbeknownst to me this line in the parent CMakeLIsts.txt file was interfering:
https://github.com/llvm/llvm-test-suite/blob/master/SingleSource/UnitTests/CMakeLists.txt#L15

I just moved up the Fortran test to be enabled before this line.

Meinersbur accepted this revision.Jan 6 2021, 2:41 PM

LGTM.

I expected more changes to be necessary, to make it work with litsupport modules, perf/timeit, statistics, compile time (Interestingly, CMAKE_Fortran_COMPILE_OBJECT is already defined), LNT, ... . But if we find they don't work yet, we can fix that later.

This revision is now accepted and ready to land.Jan 6 2021, 2:41 PM
naromero77 closed this revision.Jan 6 2021, 3:28 PM