[Fortran] Ignore whitespace in FCVS test results

[rovka]

Summary

Some tests in the FCVS suite use list-directed output, which may
according to the standard introduce arbitrary amounts of whitespace.
This patch adds the "-i" flag to the fpcmp invocation for those tests,
so that it ignores whitespace. The other tests in the suite will
continue to check whitespace strictly.

[kiranchandramohan]

Looks OK. Assuming there are no tests that explicitly check for spacing. What is FPCMP usually set to?

[rovka]

In D128260#3600398, @kiranchandramohan wrote:

Looks OK. Assuming there are no tests that explicitly check for spacing. What is FPCMP usually set to?

I think it is usually set to fpcmp-target, which is built from this source.

I don't know what to say about the spacing. FCVS uses a mix of formatted and list-directed output. If we choose to care about the spacing we should probably move away from list-directed output. I'm not sure how much work that would be.

EDIT: Only FM905 and FM907 use list-directed output. I'll try to migrate them to formatted output.

[kiranchandramohan]

@rovka Would you know what is the scope of the llvm-testsuite? Is it limited to flang, or is it flang+gfortran, or is it any fortran compiler? And is it possible to have multiple outputs for comparison as @clementval mentioned?

[rovka]

In D128260#3604020, @kiranchandramohan wrote:

@rovka Would you know what is the scope of the llvm-testsuite? Is it limited to flang, or is it flang+gfortran, or is it any fortran compiler? And is it possible to have multiple outputs for comparison as @clementval mentioned?

It's not limited to a specific compiler, it "should work" with any compiler. We're currently running it on one of the buildbots with flang-to-external-fc, and I'd like to switch it to use flang-new instead. However, I think it's valuable to be able to run with both flang and gfortran, so we can easily compare/debug things.
At the moment I'm not aware of a way to add multiple outputs for comparison, I don't see that in any of the suites included (but I could be missing something).

[rovka]

One thing that we could do is to only enable whitespace for the 2 tests that use list-directed output. That's going to require a bit more effort in the CMakeLists.txt, but it would let us be strict about whitespace where specific formatting is used, and more relaxed where it isn't.

[kiranchandramohan]

In D128260#3604151, @rovka wrote:

One thing that we could do is to only enable whitespace for the 2 tests that use list-directed output. That's going to require a bit more effort in the CMakeLists.txt, but it would let us be strict about whitespace where specific formatting is used, and more relaxed where it isn't.

That sounds fine to me.

[awarzynski]

In D128260#3604020, @kiranchandramohan wrote:

And is it possible to have multiple outputs for comparison as @clementval mentioned?

I'm not too fond of the idea - that would require more bookkeeping and code to maintain. My preference would be to avoid this. Unless llvm-test-suite already has some logic for this (I'm not aware of such).

One thing that we could do is to only enable whitespace for the 2 tests that use list-directed output.

You meant "disable" , right?

Either approach sounds fine to me - I would also be happy with updating FM905 and FM907.

[rovka]

Right, disable :)
Updated the patch to special-case FM905 and FM907. I think it doesn't hurt to leave a bit of list-directed output in the test suite just to make sure it doesn't randomly explode.

[kiranchandramohan]

The patch application failed.
Do we need to update the README file with the change?

In D128260#3604080, @rovka wrote:

In D128260#3604020, @kiranchandramohan wrote:

@rovka Would you know what is the scope of the llvm-testsuite? Is it limited to flang, or is it flang+gfortran, or is it any fortran compiler? And is it possible to have multiple outputs for comparison as @clementval mentioned?

It's not limited to a specific compiler, it "should work" with any compiler. We're currently running it on one of the buildbots with flang-to-external-fc, and I'd like to switch it to use flang-new instead. However, I think it's valuable to be able to run with both flang and gfortran, so we can easily compare/debug things.
At the moment I'm not aware of a way to add multiple outputs for comparison, I don't see that in any of the suites included (but I could be missing something).

Because there are a lot of things that are not standardised in Fortran, we might keep running into these issues. Since ifx/ifort also differs from flang/gfortran, the goal of working with any compiler is probably difficult to achieve in Fortran I think.

In D128260#3604620, @awarzynski wrote:

In D128260#3604020, @kiranchandramohan wrote:

And is it possible to have multiple outputs for comparison as @clementval mentioned?

I'm not too fond of the idea - that would require more bookkeeping and code to maintain. My preference would be to avoid this. Unless llvm-test-suite already has some logic for this (I'm not aware of such).

Yes, it might not be worth the trouble if there is no existing support for this. Otherwise, the changes will be localised to the reference file.

cmake/modules/SingleMultiSource.cmake

108–117

I am not pushing for this option. But this bit here seems to allow for reference output based on TARGET, ENDIAN-NESS etc.

[rovka]

Try to fix CI patch application.

[rovka]

Try to fix CI patch application - take 2.

[rovka]

In D128260#3604911, @kiranchandramohan wrote:

The patch application failed.

Right, not sure why it doesn't find those files, I'll try a few things...

Do we need to update the README file with the change?

I don't think so, since this doesn't affect the FCVS files themselves.

In D128260#3604080, @rovka wrote:

In D128260#3604020, @kiranchandramohan wrote:

@rovka Would you know what is the scope of the llvm-testsuite? Is it limited to flang, or is it flang+gfortran, or is it any fortran compiler? And is it possible to have multiple outputs for comparison as @clementval mentioned?

It's not limited to a specific compiler, it "should work" with any compiler. We're currently running it on one of the buildbots with flang-to-external-fc, and I'd like to switch it to use flang-new instead. However, I think it's valuable to be able to run with both flang and gfortran, so we can easily compare/debug things.
At the moment I'm not aware of a way to add multiple outputs for comparison, I don't see that in any of the suites included (but I could be missing something).

Because there are a lot of things that are not standardised in Fortran, we might keep running into these issues. Since ifx/ifort also differs from flang/gfortran, the goal of working with any compiler is probably difficult to achieve in Fortran I think.

In D128260#3604620, @awarzynski wrote:

In D128260#3604020, @kiranchandramohan wrote:

And is it possible to have multiple outputs for comparison as @clementval mentioned?

I'm not too fond of the idea - that would require more bookkeeping and code to maintain. My preference would be to avoid this. Unless llvm-test-suite already has some logic for this (I'm not aware of such).

Yes, it might not be worth the trouble if there is no existing support for this. Otherwise, the changes will be localised to the reference file.

cmake/modules/SingleMultiSource.cmake

108–117

Right, I guess we could add some logic here based on COMPILER_ID or something of the sort.

[awarzynski]

Fortran/UnitTests/fcvs21_f95/CMakeLists.txt

36–42

This is basically splitting the tests into 2 groups, which are processed independently. I think that it would help to have this highlighted a bit more. Perhaps something like this:

# GROUP 1:
# Tests 905 and 907 use list-directed output, for which the standard allows some flexibility with regards to how many white space characters to print. For these tests, we ignore whitespaces when comparing the output. 
set(SPECIAL_CASES "FM905.f" "FM907.f")
set(Source ${SPECIAL_CASES})

set(FP_IGNOREWHITESPACE ON)
llvm_singlesource()

# GROUP 2:
# Generic case.
set(Source)

cmake/modules/SingleMultiSource.cmake

108–117

+1 I'm OK to revisit this once it becomes a problem (as opposed to expanding this patch)

[rovka]

Added more comments.
Hopefully the pre-merge CI will actually run this time...

Fortran/UnitTests/fcvs21_f95/CMakeLists.txt

36–42

Makes sense :)

cmake/modules/SingleMultiSource.cmake

108–117

Cool, thanks!

[rovka]

No change, trying to convince the pre-merge to apply the patch...

[awarzynski]

In D128260#3614645, @rovka wrote:

No change, trying to convince the pre-merge to apply the patch...

How confident are you that it should work? I mean, perhaps the pre-merge CI has not been configured correctly for llvm-test-suite? I've just tried arc patch D128260 and it worked fine for me locally. This looks like the pre-merge CI issue. Perhaps create a bug?

To me all this makes sense and hence approving as is, thanks! AFAIK, the only buildbots running these tests are managed by Linaro, so you will know if something is broken :)

EDIT Same issue with patch application: https://reviews.llvm.org/D126453 (this is quite recent)

[rovka]

In D128260#3614974, @awarzynski wrote:

In D128260#3614645, @rovka wrote:

No change, trying to convince the pre-merge to apply the patch...

How confident are you that it should work? I mean, perhaps the pre-merge CI has not been configured correctly for llvm-test-suite? I've just tried arc patch D128260 and it worked fine for me locally. This looks like the pre-merge CI issue. Perhaps create a bug?

To me all this makes sense and hence approving as is, thanks! AFAIK, the only buildbots running these tests are managed by Linaro, so you will know if something is broken :)

Yep, I'll keep an eye out :)

EDIT Same issue with patch application: https://reviews.llvm.org/D126453 (this is quite recent)

[rovka]

In D128260#3614974, @awarzynski wrote:

In D128260#3614645, @rovka wrote:

No change, trying to convince the pre-merge to apply the patch...

How confident are you that it should work? I mean, perhaps the pre-merge CI has not been configured correctly for llvm-test-suite? I've just tried arc patch D128260 and it worked fine for me locally. This looks like the pre-merge CI issue. Perhaps create a bug?

I commented on https://github.com/google/llvm-premerge-checks/issues/329

Fortran/UnitTests/fcvs21_f95/CMakeLists.txt

 # 
 # Note that the NIST license allows redistribution but requires
 # explicit acknowledgement of what has been changed. Please
 # amend README file if you modify the tests.
 
 # clean-up fort.* files otherwise one of the tests will fail
 llvm_test_prepare(rm -f %S/fort.*)
 
+# Group 1:
+# Tests 905 and 907 use list-directed output, for which the standard allows some
+# flexibility, e.g.,  with regards to how many white space characters or
+# floating point decimals to print. Treat them separately.
+set(SPECIAL_CASES "FM905.f" "FM907.f")
+set(Source ${SPECIAL_CASES})
+set(FP_IGNOREWHITESPACE ON)
+llvm_singlesource()
+set(Source)
+
+# Group 2:
+# Generic case. Make sure to exclude the special cases which have already been
+# added as part of Group 1.
 file(GLOB Source CONFIGURE_DEPENDS *.f)
+foreach (SPECIAL_CASE ${SPECIAL_CASES})
+  list(FILTER Source EXCLUDE REGEX ${SPECIAL_CASE})
+endforeach()
 
 # Test 509 requires this flag in more recent versions of GCC.
 # If this flag is not supported, the test should be excluded.
 check_fortran_compiler_flag("-std=legacy" SUPPORTS_LEGACY)
 if (SUPPORTS_LEGACY)
   add_compile_options(-std=legacy)
 else()
   # Regex because the GLOB returns the full path to each file
   list(FILTER Source EXCLUDE REGEX "FM509\.f$")
 endif()
 
 set(FP_TOLERANCE 1.0e-11) # set by the most sensitive numerical test
+set(FP_IGNOREWHITESPACE OFF)
 llvm_singlesource()
 
 file(COPY lit.local.cfg DESTINATION "${CMAKE_CURRENT_BINARY_DIR}")

cmake/modules/SingleMultiSource.cmake

if(NOT NO_REFERENCE_OUTPUT)
     if(SMALL_PROBLEM_SIZE)
       set(SIZE_SUFFIX small)
     elseif(LARGE_PROBLEM_SIZE)
       set(SIZE_SUFFIX large)
     else()
       set(SIZE_SUFFIX)
     endif()
 
     if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian.${SIZE_SUFFIX})
       set(REFERENCE_OUTPUT ${name}.reference_output.${ENDIAN}-endian.${SIZE_SUFFIX})
     elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${SIZE_SUFFIX})
       set(REFERENCE_OUTPUT ${name}.reference_output.${SIZE_SUFFIX})
     elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${ENDIAN}-endian)
       set(REFERENCE_OUTPUT ${name}.reference_output.${ENDIAN}-endian)
     elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output.${TARGET_OS})
       set(REFERENCE_OUTPUT ${name}.reference_output.${TARGET_OS})
     elseif(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${name}.reference_output)
       set(REFERENCE_OUTPUT ${name}.reference_output)
     else()
       message("-- No reference output found for test ${name}")
     endif()
   endif()
 
   if(REFERENCE_OUTPUT)
     set(DIFFPROG %b/${FPCMP})
     if(FP_TOLERANCE)
       set(DIFFPROG "${DIFFPROG} -r ${FP_TOLERANCE}")
     endif()
     if(FP_ABSTOLERANCE)
       set(DIFFPROG "${DIFFPROG} -a ${FP_ABSTOLERANCE}")
     endif()
+    if(FP_IGNOREWHITESPACE)
+      set(DIFFPROG "${DIFFPROG} -i")
+    endif()
     llvm_test_verify(${DIFFPROG} %o %S/${REFERENCE_OUTPUT})
     llvm_test_data(${target} ${REFERENCE_OUTPUT})
   endif()
   set(TESTSCRIPT "${TESTSCRIPT}" PARENT_SCOPE)
 endfunction()