Porting F18 tests to use LLVM lit without FileCheck #1027

LukeIreland1 · 2020-02-26T15:15:25Z

#941 had diverged a fair amount, and I thought making a new PR would make things a lot cleaner and simpler.

This PR ports all Fortran tests, and is a precursor for porting the remaining unit tests (currently using ctest) to use gtest instead as per #995.

The majority of these Fortran tests will later be ported to FileCheck per #996, and the remaining error tests will be ported using a flang -verify equivalent per #997.

DavidTruby · 2020-02-26T15:29:48Z

test/Evaluate/folding01.f90

@@ -1,3 +1,4 @@
+!RUN: %S/test_folding.sh %s %flang


Might it be worth adding "tools" for these in lit.cfg.py so instead of doing %S/test_folding.sh we can do %folding? It might just be a little neater than having to specify the path to the script.

Funny you should say that! I did this originally, but both @tskeith and @RichBarton-Arm said they prefer it like this (I think it was something to do with the neatness of the config file being preferable).

Yes, I still prefer it like this.

Happy to go with the majority opinion on this one, disregard my comment

DavidTruby · 2020-02-26T15:32:59Z

test/Semantics/critical04.f90

@@ -1,4 +1,5 @@
-! RUN: ${F18} -funparse-with-symbols %s 2>&1 | ${FileCheck} %s
+!RUN: %S/test_any.sh %s %flang
+! EXEC: ${F18} -funparse-with-symbols %s 2>&1 | ${FileCheck} %s


What does EXEC: do here? I've not seen it used anywhere in LLVM before.
Should this say %f18 instead of ${F18}?

It's identical, but used to make their scripts work. test_any.sh uses identical runlines, so we replaced them with EXEC lines, and left its version of %f18 as it works without being changed.

test_any.sh sort of behaves like lit and FileCheck put together. So the tests have RUN: lines in them which generally run F18 and pipe through $FileCheck which is an internal function in test_any.sh (and can be substituted for real FileCheck).

We agreed on the last PR that we would stick strictly to our plan of porting to lit first using the existing scripts then port off the scripts one by one, just to keep things separated. So our slightly hacky solution was to rewrite test_any.sh to look for EXEC lines instead so lit could have the RUN lines. I think test_any.sh is the first script we should port over to FileCheck under #996 to make this look nicer.

test-lit/Semantics/common.sh

tskeith · 2020-02-26T15:59:21Z

I don't see how I can review this set of commits. I can't look at all of them together because it's a mix of hand-written and mechanical changes but if I look at them individually there are all kinds or extraneous file moves. Those moves don't need to be visible in the final commits.

tskeith · 2020-02-26T17:04:16Z

When I run ninja check-all I get an error because it's looking for test-lit/lit.cfg.py.

RichBarton-Arm · 2020-02-26T18:30:53Z

I don't see how I can review this set of commits. I can't look at all of them together because it's a mix of hand-written and mechanical changes but if I look at them individually there are all kinds or extraneous file moves. Those moves don't need to be visible in the final commits.

Understood. Luke took this feedback from the last review and for this one he separated the mechanical changes from the automated changes from the manual changes in separate commits so I recommend reviewing commit by commit.

The first patch in the series introduces the porting script and moves the test scripts to the new locations that the porting script expects them to be.
The second and third patches are hand-written changes to the existing tests with rationale
The fourth commit is simply the result of running the script on the remaining Fortran ctest tests and porting them to lit style.
The fifth and sixth patches are mechanical renames/deletions of files.

Hope that helps with the review. If you can propose another arrangement of patches that would make the review easier then @LukeIreland1 can rearrange accordingly.

tskeith · 2020-02-26T23:52:01Z

Hope that helps with the review. If you can propose another arrangement of patches that would make the review easier then @LukeIreland1 can rearrange accordingly.

Here's what I said in the the other PR:

That sounds good to me, though before approving I would like the see the commits in the form that they will be merged. I assume there will be no evidence of the porting script or test-lit. Just removing ctest configuration, adding lit configuration, and 1-line (or so) changes to each test source file. And maybe some changes to the shell scripts.

RichBarton-Arm · 2020-02-27T08:01:35Z

@tskeith Some of the tests needed more effort to port than a one-line change so there has to be some hand-written changes in this PR. These changes are separated out in patches #2 and #3 for specific review. Additionally, some changes were needed to the testing scripts themselves to make them interact well with lit and these are in patch #1. The file moves and mechanical file moves with a one-line diff are in patches #4-#6.

That sounds good to me, though before approving I would like the see the commits in the form that they will be merged. I assume there will be no evidence of the porting script or test-lit. Just removing ctest configuration, adding lit configuration, and 1-line (or so) changes to each test source file. And maybe some changes to the shell scripts.

Unless I misunderstand your request, isn't this the same as reviewing all commits together? https://github.com/flang-compiler/f18/pull/1027/files, modulo a commit removing the script at the end?

My recommendation is to review patches #1, #2 and #3 like normal code changes then review the rest as a single lump bearing in mind most of it is mechanical. Does that seem like a way forward?

LukeIreland1 · 2020-02-27T13:27:42Z

Rebased on top of master

tskeith · 2020-02-27T15:51:11Z

test-lit/Semantics/common.sh

-[[ $KEEP ]] || trap "rm -rf $temp" EXIT
+F18=$2
+temp=$3
+mkdir -p $temp


This used to report an error if the wrong number of arguments is passed in. That seems like a good thing.

The wrong number of arguments can't be passed in (so it's not needed) as far as the automated tests are concerned, I'm sure people manually using the tests will know what they're doing (but they shouldn't since new tests should be gtest or lit), and the main reason I took this out was to be able to add %t as an argument, and I couldn't otherwise.

tskeith · 2020-02-27T16:05:52Z

test/Preprocessing/pp001.F

@@ -1,3 +1,4 @@
+!RUN: %flang -E %s


These files were tests before. I'm not sure this adds much because there is no checking of results.

They don't need to be checked in this case.

Sorry, I meant to write "these files were not tests before". So running them as if they are tests is a change. I'm wondering if there is any benefit to doing that.

I suppose not, but we thought to port them unless there is an objection since their addition implies they will be included as a test in future.

Tests for the preprocessor should not need to compile to an executing image that prints pass/fail. It should be possible to test the preprocessor by running FileCheck on the output of -E. So I think the long-term future of these tests is that they are re-written in terms of FileCheck and -E.

We think our short-term options are

Delete them as they are not running tests at the moment. Add new preprocessor testing back in later in a new style.

Port them over as best we can and fix up later.

Luke has gone for option 2 and ported them as tests that just run to ensure the compiler can consume the input. We'd have no codegen, so we can't compile and run the tests at the moment so we need to run them under an option to terminate the compiler at an earlier phase. -E seems like the most sensible as this is what we would use in the final version.

As a later exercise, someone can rewrite these tests using FileCheck and -E and assess them as a whole to ensure they have good coverage of the preprocessor and no redundancy.

The third option is to leave them as they were, just examples that are not executed as tests.

If you think there is value in executing them to check there are no compilation errors, that's fine. But that should be made explicit in the PR (or a separate PR) saying that's what you're doing that and why. Don't bury it under all of the changes related to porting tests.

Sorry for not being more clear in the description.

Lit will emit an error for test files in the test directory (identified by file extension) that contain no runlines. So to port these tests to lit, we have three options:

Add RUN lines to them (our chosen option)

Explicitly exclude this directory from lit's searching process

add a lit.local.cfg in this test directory that changes the file extensions associated with tests.

Rename the file extensions (yuk!)

If we don't like any of these options we could leave them behind in the unittests directory with no change and revisit later or we could delete them altogether.

To me, it seems pointless and unhelpful to have test files in the test directory that look like tests but are intentionally not executed. If we don't want to execute these then I think we should delete them. Strong view weakly held though. What would you be happy with?

1 and 2 are both fine with me, as is moving them to a different directory, say examples. I just want it to be an explicit decision that is made, not something that happens accidentally.

@klausler, what do you think we should do with the preprocessor tests?

tskeith · 2020-02-27T16:24:42Z

Unless I misunderstand your request, isn't this the same as reviewing all commits together? https://github.com/flang-compiler/f18/pull/1027/files, modulo a commit removing the script at the end?

No, I'm asking for all of the mechanical changes to be together so that the history does not contain extraneous moves to test-lit and then back again.

My recommendation is to review patches #1, #2 and #3 like normal code changes then review the rest as a single lump bearing in mind most of it is mechanical. Does that seem like a way forward?

Yes, that helps for reviewing, but my request for the final form of the commits still applies.

The changes to CMakeLists.txt don't belong with the mechanical changes.

LukeIreland1 · 2020-02-27T18:04:39Z

Unless I misunderstand your request, isn't this the same as reviewing all commits together? https://github.com/flang-compiler/f18/pull/1027/files, modulo a commit removing the script at the end?

No, I'm asking for all of the mechanical changes to be together so that the history does not contain extraneous moves to test-lit and then back again.

That's fine, 5 and 6 can be combined.

My recommendation is to review patches #1, #2 and #3 like normal code changes then review the rest as a single lump bearing in mind most of it is mechanical. Does that seem like a way forward?

Yes, that helps for reviewing, but my request for the final form of the commits still applies.

The changes to CMakeLists.txt don't belong with the mechanical changes.

Even if the change to CMakeLists.txt is purely mechanical (or purely as a result of mechanical changes)?

RichBarton-Arm · 2020-02-28T09:34:53Z

test/Preprocessing/pp001.F

@@ -1,3 +1,4 @@
+!RUN: %flang -E %s


Tests for the preprocessor should not need to compile to an executing image that prints pass/fail. It should be possible to test the preprocessor by running FileCheck on the output of -E. So I think the long-term future of these tests is that they are re-written in terms of FileCheck and -E.

We think our short-term options are

Delete them as they are not running tests at the moment. Add new preprocessor testing back in later in a new style.

Port them over as best we can and fix up later.

Luke has gone for option 2 and ported them as tests that just run to ensure the compiler can consume the input. We'd have no codegen, so we can't compile and run the tests at the moment so we need to run them under an option to terminate the compiler at an earlier phase. -E seems like the most sensible as this is what we would use in the final version.

As a later exercise, someone can rewrite these tests using FileCheck and -E and assess them as a whole to ensure they have good coverage of the preprocessor and no redundancy.

RichBarton-Arm · 2020-02-28T09:38:28Z

test/Evaluate/test_folding.sh

-mkdir $temp
+temp=$(echo "$src" | cut -f 1 -d '.')
+temp="Outputs/$temp"
+mkdir -p $temp


Shouldn't this be mkdir $3 ? You are passing in a %t on the lit RUN line but then the script goes and makes it's own tempdir and ignores the %t you passed in. The other scripts use common.sh to parse the commandline but this script does not.

RichBarton-Arm · 2020-02-28T09:39:32Z

test/Evaluate/test_folding.sh

-mkdir $temp
+temp=$(echo "$src" | cut -f 1 -d '.')
+temp="Outputs/$temp"
+mkdir -p $temp
 [[ $KEEP ]] || trap "rm -rf $temp" EXIT


Related to above comment - if you are passing in the temp dir from lit then you don't need to delete it here. Lit will handle that for you.

RichBarton-Arm · 2020-02-28T09:42:17Z

test/Evaluate/folding01.f90

@@ -1,3 +1,4 @@
+!RUN: %S/test_folding.sh %s %flang %t


I think this would look nicer like ! RUN (i.e. with a space after the comment char) I don't think there is a strict style guide anywhere that says this but most normal comments would start with this space for readability.

RichBarton-Arm · 2020-02-28T09:48:18Z

test/Semantics/critical04.f90

@@ -1,4 +1,5 @@
-! RUN: ${F18} -funparse-with-symbols %s 2>&1 | ${FileCheck} %s
+!RUN: %S/test_any.sh %s %flang
+! EXEC: ${F18} -funparse-with-symbols %s 2>&1 | ${FileCheck} %s


test_any.sh sort of behaves like lit and FileCheck put together. So the tests have RUN: lines in them which generally run F18 and pipe through $FileCheck which is an internal function in test_any.sh (and can be substituted for real FileCheck).

We agreed on the last PR that we would stick strictly to our plan of porting to lit first using the existing scripts then port off the scripts one by one, just to keep things separated. So our slightly hacky solution was to rewrite test_any.sh to look for EXEC lines instead so lit could have the RUN lines. I think test_any.sh is the first script we should port over to FileCheck under #996 to make this look nicer.

RichBarton-Arm · 2020-02-28T10:02:12Z

test/Semantics/common.sh

-[[ $KEEP ]] || trap "rm -rf $temp" EXIT
+F18=$2
+temp=$3
+mkdir -p $temp


github wants you to add a newline here.

RichBarton-Arm · 2020-02-28T10:12:06Z

unittests/Evaluate/CMakeLists.txt

@@ -134,15 +134,6 @@ target_link_libraries(folding-test
 )

 set(FOLDING_TESTS


The empty list should also be removed, as well as any references to it lower down the file.

RichBarton-Arm · 2020-02-28T10:29:14Z

Unless I misunderstand your request, isn't this the same as reviewing all commits together? https://github.com/flang-compiler/f18/pull/1027/files, modulo a commit removing the script at the end?

No, I'm asking for all of the mechanical changes to be together so that the history does not contain extraneous moves to test-lit and then back again.

My recommendation is to review patches #1, #2 and #3 like normal code changes then review the rest as a single lump bearing in mind most of it is mechanical. Does that seem like a way forward?

Yes, that helps for reviewing, but my request for the final form of the commits still applies.

Perhaps the way forward is to review this change as a patch series and agree we are happy with it, then for Luke to squash the series into two commits:

the patch which fixes test/Semantics/altreturn{02,02}
a single commit that does the rest - which would look like tests moving out of test into unittests with CMakeLists.txt changes; tests in test changing to use lit and accompanying CMakeLists changes and content moving over from test-lit and merging with content in test

We can do a final review on that patch at the time. Bear in mind that if more new ctest tests are added between Luke creating the PR and it getting approved then we'd need to create a new one as it will likely not merge properly. So we'll need to synchronise on that.

Does that sound ok to you?

The changes to CMakeLists.txt don't belong with the mechanical changes.

We wrote the script to remove ported tests from the CMakeLists files in test as it goes along. This is why it ends up in the same patch. We found this was useful to ensure we didn't miss anything along the way. We can easily change the script behaviour and make the CMakeLists.txt changes manually in a separate patch in the series if you have a strong preference for that.

tskeith · 2020-02-29T03:32:23Z

test-lit/Semantics/omp-nested01.f90

@@ -1,3 +1,5 @@
+!RUN: %S/test_errors.sh %s %flang %t
+!XFAIL: *


XFail? This test is commented out, so we ported it, but left it as a failing test.

LukeIreland1 · 2020-03-02T13:00:47Z

Rebased on top of master

tskeith · 2020-03-03T01:11:21Z

Perhaps the way forward is to review this change as a patch series and agree we are happy with it, then for Luke to squash the series into two commits:
* the patch which fixes test/Semantics/altreturn{02,02}

* a single commit that does the rest - which would look like tests moving out of `test` into `unittests` with CMakeLists.txt changes; tests in `test` changing to use lit and accompanying CMakeLists changes and content moving over from `test-lit` and merging with content in `test`

How about this set of commits:

fix altreturn tests
move tests to unittests
add "RUN:" lines to tests that will be ported to lit (i.e. the change that no one will look at because it is a similar change to hundreds of files)
changes to run tests with lit rather than ctest
move tests from test-lit to test

LukeIreland1 · 2020-03-05T14:09:02Z

Rebased on top of master.

LukeIreland1 · 2020-03-05T15:14:16Z

@sscalpone @tskeith @klausler Do any of you have any further requests for this change? It's ready to merge other than that.

tskeith · 2020-03-05T15:28:16Z

@sscalpone @tskeith @klausler Do any of you have any further requests for this change? It's ready to merge other than that.

This isn't okay. The history of every test file will show it moving to test-lit and back to test again.

LukeIreland1 · 2020-03-05T15:47:16Z

@sscalpone @tskeith @klausler Do any of you have any further requests for this change? It's ready to merge other than that.

This isn't okay. The history of every test file will show it moving to test-lit and back to test again.

Then as @RichBarton-Arm suggested, the last 4 can be combined. Although, I'm not sure I understand why this is an issue.

Would you like me to combine them already, or is there something about a specific commit you want changed? For now, I can only think of deleting the old tests in the same commit as the move, which will make the move commit look like the porting commit, and make the series pointless.

LukeIreland1 · 2020-03-06T14:50:39Z

@tskeith Are those 4 commits any better? They can be squashed further.

RichBarton-Arm

You have not addressed my comment about dead code on the first patch. More comments on the final patch too.

Can you give me an example lit commandline where you have enabled testing with libpgmath? None of the below worked for me:

LIBPGMATH=1 ./build/llvm/bin/llvm-lit build/f18/test/ -a -vv |& tee test.log
LIBPGMATH=True ./build/llvm/bin/llvm-lit build/f18/test/ -a -vv |& tee test.log
./build/llvm/bin/llvm-lit build/f18/test/ --param=LIBPGMATH="True" -a -vv |& tee test.log
./build/llvm/bin/llvm-lit build/f18/test/ --param=LIBPGMATH=1 -a -vv |& tee test.log

RichBarton-Arm · 2020-03-11T11:27:00Z

test/Semantics/common.sh

@@ -10,19 +10,18 @@ function die {
  echo "$(basename $0): $*" >&2
  exit 1
 }
+if [[ $# < 1 ]]; then
+  echo "Usage: $(basename $0) <fortran-source> [<f18-executable>] <temp test dir>"


The [ ] syntax in the usage string is meant to indicate an optional argument. In the new interface, the F18 executable is non-optional so these should be removed. I think all the arguments are non-optional in the new interface.

RichBarton-Arm · 2020-03-11T11:27:34Z

test/Evaluate/test_folding.sh


 if [[ $# < 1 ]]; then
-  echo "Usage: $0 <fortran-source> [-pgmath=<true/false>]"
+  echo "Usage: $0 <fortran-source> [<f18-executable>] <temp test dir>"


Same comment as on common.sh. is no longer an optional argument, so lose the [] notation.

RichBarton-Arm · 2020-03-11T11:29:21Z

test/Semantics/common.sh

@@ -10,19 +10,18 @@ function die {
  echo "$(basename $0): $*" >&2
  exit 1
 }
+if [[ $# < 1 ]]; then


This logic is wrong. This means that only an invocation with no arguments will cause the usage string to be printed. Given that now our interface mandates 3 arguments it would be friendlier to print the usage string if less than 3 arguments are given.

RichBarton-Arm · 2020-03-11T11:30:04Z

test/Evaluate/test_folding.sh

-srcdir=$(dirname $0)
-F18CC=${F18:-../../../tools/f18/bin/f18}
-CMD="$F18CC -fdebug-dump-symbols -fparse-only"
+CMD="$2 -fdebug-dump-symbols -fparse-only"

 if [[ $# < 1 ]]; then


Same comment as common.sh. The minimum number of arguments has changed to 3, so we should print usage for less than 3.

RichBarton-Arm · 2020-03-11T11:31:13Z

test/Preprocessing/lit.local.cfg.py

+import lit.formats
+from lit.llvm import llvm_config
+
+# Added this lit config file to prevent lit from discovering these tests


Pedantry, but the presence of the file itself does not do this. It is the assignment to config.suffixes that does it. Change the comment to explain that.

RichBarton-Arm · 2020-03-11T11:31:22Z

test/Preprocessing/lit.local.cfg.py

@@ -0,0 +1,12 @@
+# -*- Python -*-
+
+import os


Do you still need these imports? (Genuine question)

RichBarton-Arm

I think this is basically good, just a couple of NFC changes (in comments) and I will approve.

RichBarton-Arm · 2020-03-13T11:09:14Z

test/lit.cfg.py

@@ -72,7 +64,11 @@
 tools = [ToolSubst('%flang', command=FindTool('flang'), unresolved='fatal'),
         ToolSubst('%f18', command=FindTool('f18'), unresolved='fatal'),
         ToolSubst('%f18_with_includes', command=FindTool('f18'),
-         extra_args=[flang_includes], unresolved='fatal')]
+                   extra_args=[flang_includes], unresolved='fatal')]


This change in indentation is not needed

RichBarton-Arm · 2020-03-13T11:10:06Z

unittests/Evaluate/CMakeLists.txt

@@ -140,11 +140,3 @@ add_test(Real real-test)
 add_test(RESHAPE reshape-test)
 add_test(ISO-binding ISO-Fortran-binding-test)
 add_test(folding folding-test)
-


This should be part of patch #1

These tests were disabled due to flang-compiler#407. Previously these tests caused F18 to crash as the feature was not fully implemented. The altreturn feature is now implemented, so these tests can be re-enabled. altreturn03 tested some negative cases which F18 correctly diagnoses. Modified that test to expect these new error messages. Also make the later cases in the test reachable. These tests can now be ported by the script to lit-style tests. Change-Id: Ib336c10d55068d9a26fc2deb43ad052e74e73456

Some of the regression tests are C programs that act as test harnesses for the compiler internals as opposed to being Fortran inputs to test the compiler in action. The former style of tests are analog to LLVM's unittests and will not use the lit framework. Change-Id: I0ff10e23f66ff843e8fff4c35cfb6559b9dab762

We have re-classified a subset of the regression tests as unit tests and now we are porting the remaining ones. Test discovery and running is now performed by lit rather than ctest. The tests continue to use their original scripts with minor modifications. Most of the changes were mechanical and so scripted. A few changes were made by hand. Details Manual: * modfile09-*.f90 tests depend on being run together as some tests have dependencies on modules created by other tests. This will need separating out when porting away from test_modfile.sh, but for now, added modfile09-*.f90 to the Inputs directory and added a single tests modfile09.f90 to hold the run line. * getdefinition03-a.f90 includes a non-test file getdefinition03-b.f90. Manually edited the former to find the latter in Inputs so as to add only one test. * Same pattern for getsymbols03-{a,b}.f90 Auto: * Remaining tests have a lit RUN line added to them based on the type of test they are. * Failing tests also have an XFAIL line added to them. * Generic tests have their pre-existing RUN lines replaced with the word "EXEC" to avoid conflict with the added lit RUN line.

RichBarton-Arm · 2020-03-13T12:21:29Z

test/lit.cfg.py

-  config.available_features.add('fir')
-  if config.llvm_tools_dir != config.flang_llvm_tools_dir :
-    llvm_config.with_environment('PATH', config.flang_llvm_tools_dir, append_path=True)
+if config.flang_llvm_tools_dir != "":


Again, please don't make random formatting changes that are unrelated to the patch.

All Fortran tests are now run in lit, except Preprocessing tests flang-compiler#1052 Preprocessing tests are a separate kind of test, so will be sorted out later.

RichBarton-Arm

LGTM - we finally got there.

tskeith

Looks good

schweitzpgi · 2020-03-17T22:43:59Z

test/CMakeLists.txt

+  list(APPEND FLANG_TEST_DEPENDS tco)
+endif()
+
+add_lit_testsuite(check-all "Running the Flang regression tests"


This is a bug and only works when compiling f18 standalone. LLVM already has a cmake target of "check-all" and this will conflict with it.

I've fixed this already in the cmake rework PR. Check-all will still exist as a if flang is built out of tree running only flang tests, and the check-flang target will always exist and always only run flang tests.

These tests were disabled due to flang-compiler/f18#407. Previously these tests caused F18 to crash as the feature was not fully implemented. The altreturn feature is now implemented, so these tests can be re-enabled. altreturn03 tested some negative cases which F18 correctly diagnoses. Modified that test to expect these new error messages. Also make the later cases in the test reachable. These tests can now be ported by the script to lit-style tests. Change-Id: Ib336c10d55068d9a26fc2deb43ad052e74e73456 Original-commit: flang-compiler/f18@4de19d7 Reviewed-on: flang-compiler/f18#1027 Tree-same-pre-rewrite: false

Some of the regression tests are C programs that act as test harnesses for the compiler internals as opposed to being Fortran inputs to test the compiler in action. The former style of tests are analog to LLVM's unittests and will not use the lit framework. Change-Id: I0ff10e23f66ff843e8fff4c35cfb6559b9dab762 Original-commit: flang-compiler/f18@2bfddbe Reviewed-on: flang-compiler/f18#1027 Tree-same-pre-rewrite: false

We have re-classified a subset of the regression tests as unit tests and now we are porting the remaining ones. Test discovery and running is now performed by lit rather than ctest. The tests continue to use their original scripts with minor modifications. Most of the changes were mechanical and so scripted. A few changes were made by hand. Details Manual: * modfile09-*.f90 tests depend on being run together as some tests have dependencies on modules created by other tests. This will need separating out when porting away from test_modfile.sh, but for now, added modfile09-*.f90 to the Inputs directory and added a single tests modfile09.f90 to hold the run line. * getdefinition03-a.f90 includes a non-test file getdefinition03-b.f90. Manually edited the former to find the latter in Inputs so as to add only one test. * Same pattern for getsymbols03-{a,b}.f90 Auto: * Remaining tests have a lit RUN line added to them based on the type of test they are. * Failing tests also have an XFAIL line added to them. * Generic tests have their pre-existing RUN lines replaced with the word "EXEC" to avoid conflict with the added lit RUN line. Original-commit: flang-compiler/f18@63ec0af Reviewed-on: flang-compiler/f18#1027 Tree-same-pre-rewrite: false

…compatible. All Fortran tests are now run in lit, except Preprocessing tests flang-compiler/f18#1052 Preprocessing tests are a separate kind of test, so will be sorted out later. Original-commit: flang-compiler/f18@3f99d35 Reviewed-on: flang-compiler/f18#1027

…master Porting F18 tests to use LLVM lit without FileCheck Original-commit: flang-compiler/f18@89bc84f Reviewed-on: flang-compiler/f18#1027

These tests were disabled due to flang-compiler/f18#407. Previously these tests caused F18 to crash as the feature was not fully implemented. The altreturn feature is now implemented, so these tests can be re-enabled. altreturn03 tested some negative cases which F18 correctly diagnoses. Modified that test to expect these new error messages. Also make the later cases in the test reachable. These tests can now be ported by the script to lit-style tests. Change-Id: Ib336c10d55068d9a26fc2deb43ad052e74e73456 Original-commit: flang-compiler/f18@4de19d7 Reviewed-on: flang-compiler/f18#1027 Tree-same-pre-rewrite: false

Some of the regression tests are C programs that act as test harnesses for the compiler internals as opposed to being Fortran inputs to test the compiler in action. The former style of tests are analog to LLVM's unittests and will not use the lit framework. Change-Id: I0ff10e23f66ff843e8fff4c35cfb6559b9dab762 Original-commit: flang-compiler/f18@2bfddbe Reviewed-on: flang-compiler/f18#1027 Tree-same-pre-rewrite: false

We have re-classified a subset of the regression tests as unit tests and now we are porting the remaining ones. Test discovery and running is now performed by lit rather than ctest. The tests continue to use their original scripts with minor modifications. Most of the changes were mechanical and so scripted. A few changes were made by hand. Details Manual: * modfile09-*.f90 tests depend on being run together as some tests have dependencies on modules created by other tests. This will need separating out when porting away from test_modfile.sh, but for now, added modfile09-*.f90 to the Inputs directory and added a single tests modfile09.f90 to hold the run line. * getdefinition03-a.f90 includes a non-test file getdefinition03-b.f90. Manually edited the former to find the latter in Inputs so as to add only one test. * Same pattern for getsymbols03-{a,b}.f90 Auto: * Remaining tests have a lit RUN line added to them based on the type of test they are. * Failing tests also have an XFAIL line added to them. * Generic tests have their pre-existing RUN lines replaced with the word "EXEC" to avoid conflict with the added lit RUN line. Original-commit: flang-compiler/f18@63ec0af Reviewed-on: flang-compiler/f18#1027 Tree-same-pre-rewrite: false

…compatible. All Fortran tests are now run in lit, except Preprocessing tests flang-compiler/f18#1052 Preprocessing tests are a separate kind of test, so will be sorted out later. Original-commit: flang-compiler/f18@3f99d35 Reviewed-on: flang-compiler/f18#1027

DavidTruby reviewed Feb 26, 2020

View reviewed changes

tskeith reviewed Feb 26, 2020

View reviewed changes

test-lit/Semantics/common.sh Outdated Show resolved Hide resolved

LukeIreland1 force-pushed the master branch from 922a1f7 to 34bc7af Compare February 27, 2020 13:26

tskeith reviewed Feb 27, 2020

View reviewed changes

RichBarton-Arm suggested changes Feb 28, 2020

View reviewed changes

tskeith reviewed Feb 29, 2020

View reviewed changes

LukeIreland1 force-pushed the master branch from 89575fe to 315a42f Compare March 2, 2020 13:00

LukeIreland1 force-pushed the master branch 2 times, most recently from c223ba0 to 4daf1e4 Compare March 5, 2020 14:07

LukeIreland1 force-pushed the master branch from 4daf1e4 to 82162b5 Compare March 5, 2020 15:14

LukeIreland1 force-pushed the master branch 3 times, most recently from b901f61 to 1180a62 Compare March 6, 2020 11:42

LukeIreland1 force-pushed the master branch from c2266f3 to 7abb6e9 Compare March 10, 2020 19:45

RichBarton-Arm reviewed Mar 11, 2020

View reviewed changes

LukeIreland1 force-pushed the master branch from 7abb6e9 to c4165a0 Compare March 11, 2020 17:52

RichBarton-Arm moved this from In progress to In review in LLVMify F18 - before merging Mar 12, 2020

RichBarton-Arm moved this from In review to In progress in LLVMify F18 - before merging Mar 12, 2020

RichBarton-Arm moved this from In progress to In review in LLVMify F18 - before merging Mar 12, 2020

LukeIreland1 force-pushed the master branch from c4165a0 to 6d33f1a Compare March 12, 2020 17:40

RichBarton-Arm suggested changes Mar 13, 2020

View reviewed changes

LukeIreland1 added 3 commits March 13, 2020 11:58

LukeIreland1 force-pushed the master branch from 6d33f1a to 5043d4a Compare March 13, 2020 11:59

RichBarton-Arm suggested changes Mar 13, 2020

View reviewed changes

Added CMakeLists changes, moved config and made test scripts compatible.

3f99d35

All Fortran tests are now run in lit, except Preprocessing tests flang-compiler#1052 Preprocessing tests are a separate kind of test, so will be sorted out later.

LukeIreland1 force-pushed the master branch from 5043d4a to 3f99d35 Compare March 13, 2020 13:57

RichBarton-Arm approved these changes Mar 13, 2020

View reviewed changes

RichBarton-Arm moved this from In review to Ready to merge in LLVMify F18 - before merging Mar 13, 2020

tskeith approved these changes Mar 13, 2020

View reviewed changes

sscalpone merged commit 89bc84f into flang-compiler:master Mar 13, 2020

LLVMify F18 - before merging automation moved this from Ready to merge to Done Mar 13, 2020

schweitzpgi reviewed Mar 17, 2020

View reviewed changes

		@@ -134,15 +134,6 @@ target_link_libraries(folding-test
		)

		set(FOLDING_TESTS

		@@ -1,3 +1,5 @@
		!RUN: %S/test_errors.sh %s %flang %t
		!XFAIL: *

Porting F18 tests to use LLVM lit without FileCheck #1027

Porting F18 tests to use LLVM lit without FileCheck #1027

Conversation

LukeIreland1 commented Feb 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tskeith commented Feb 26, 2020

tskeith commented Feb 26, 2020

RichBarton-Arm commented Feb 26, 2020

tskeith commented Feb 26, 2020

RichBarton-Arm commented Feb 27, 2020

LukeIreland1 commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tskeith commented Feb 27, 2020

LukeIreland1 commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichBarton-Arm commented Feb 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeIreland1 commented Mar 2, 2020

tskeith commented Mar 3, 2020

LukeIreland1 commented Mar 5, 2020

LukeIreland1 commented Mar 5, 2020

tskeith commented Mar 5, 2020

LukeIreland1 commented Mar 5, 2020

LukeIreland1 commented Mar 6, 2020

RichBarton-Arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichBarton-Arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichBarton-Arm left a comment

Choose a reason for hiding this comment

tskeith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeIreland1 commented Feb 26, 2020 •

edited