This is an archive of the discontinued LLVM Phabricator instance.

[Fortran/gfortran] add mechanism to track support for alternate flags
ClosedPublic

Authored by tblah on Jul 24 2023, 7:47 AM.

Details

Summary

The idea is to add feature gated denylist files to be appended to the
base denylists. The additional features are specified with cmake
-DTEST_SUITE_FORTRAN_FEATURES="FEATURE1 FEATURE2". The must be
DisabledFilesFEATURE.cmake files present.

One alternative would be to make including the
DisabledFilesFEATURE.cmake files optional. I decided not to do this
because on my system cmake takes a few seconds longer to configure when
the files to be included are optionally absent. Furthermore, it is
good to get some kind of error if the feature name is misspelt.

As an example, this patch includes denylists for HLFIR. I run with
-fstack-arrays -O3 -mcpu-native -flto -flang-experimental-hlfir, but so
far as I know the additional failures are all due to HLFIR. I would
expect the same denylists to work for just -flang-experimental-hlfir.

I expect there are many tests which are disabled by default which will
now work with HLFIR, but I haven't yet had chance to determine which
these are.

Diff Detail

Repository
rT test-suite

Event Timeline

tblah created this revision.Jul 24 2023, 7:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
tblah requested review of this revision.Jul 24 2023, 7:47 AM

Thank you for the changes, Tom!

Fortran/gfortran/CMakeLists.txt
265

It might be safer to reset the four lists *_FILES to <empty> at the beginning of each iteration.

Fortran/gfortran/regression/DisabledFilesHLFIR.cmake
20

I think this TODO is not there any more.

Just FYI, I have the following list of tests that pass with HLFIR vs FIR. I am testing with the polymorphic types support enabled - this may affect the results.

achar_4
advance_5
aliasing_dummy_5
array_constructor_51
char_cast_2
deferred_character_8
dependency_23
dependency_45
elemental_dependency_1
elemental_dependency_5
elemental_dependency_6
forall_12
forall_17
inline_transpose_1
interface_assignment_1
internal_pack_3
missing_optional_dummy_6
mvbits_4
PR37039
pr50069_1
pr68227
recursive_statement_functions
transfer_class_3
typebound_assignment_6
volatile10

Given that a test may need several features to be implemented (e.g. HLFIR + polymorphic types support), should we try to follow the per-test UNSUPPORTED/REQUIRES config as LIT does it? So instead of specifying deny lists per each feature, we would instead specify the list of required features for each test. We could probably use define_property (e.g. REQUIRES, UNSUPPORTED) and set_source_file_properties/get_source_file_property to map features lists to the source files.

I am inclined to accept this patch as is since it looks like it will be useful in the near term.

I like @vzakhari's idea of using the source file properties to add explicitly required features. I think it could also be used to enable XFAIL which is not easily done with the test suite.

@tblah, Do you expect that all the DisabledFilesFEATURE.cmake files will be empty at some point? In other words, do you expect the list of disabled tests to shrink with time? Do you think there will be permanently unsupported tests here? I am just wondering if these files are intended to be temporary while some feature is being implemented after which they will need to be removed. It makes @vzakhari's idea more appealing.

tblah added a comment.Jul 25 2023, 3:35 AM

I am inclined to accept this patch as is since it looks like it will be useful in the near term.

I like @vzakhari's idea of using the source file properties to add explicitly required features. I think it could also be used to enable XFAIL which is not easily done with the test suite.

@tblah, Do you expect that all the DisabledFilesFEATURE.cmake files will be empty at some point? In other words, do you expect the list of disabled tests to shrink with time? Do you think there will be permanently unsupported tests here? I am just wondering if these files are intended to be temporary while some feature is being implemented after which they will need to be removed. It makes @vzakhari's idea more appealing.

Yes I expect the lists to shrink with time, and then at some point HLFIR will become the default lowering and then anything remaining in these lists will be merged with the default lists.

tarunprabhu accepted this revision.Jul 25 2023, 7:08 AM

Tested on x86_64 Linux and seems to be fine.

Fortran/gfortran/CMakeLists.txt
265

These are appending to the list of *all* unsupported/unimplemented/skipped/failing lists. Clearing the lists would be incorrect.

This revision is now accepted and ready to land.Jul 25 2023, 7:08 AM
vzakhari added inline comments.Jul 25 2023, 9:05 AM
Fortran/gfortran/CMakeLists.txt
265

I was referring to UNSUPPORTED_FILES, UNIMPLEMENTED_FILES, SKIPPED_FILES and FAILING_FILES - they are reset in the CMake files included at line 265, so instead of adding phony files like Fortran/gfortran/regression/coarray/DisabledFilesHLFIR.cmake we could reset them here.

tblah added inline comments.Jul 25 2023, 9:28 AM
Fortran/gfortran/CMakeLists.txt
265

As I mentioned in the description, when I tried making the includes optional, cmake took noticeably longer to configure and then there is no error reported if a FEATURE is mispelt.

vzakhari accepted this revision.Jul 25 2023, 9:33 AM
vzakhari added inline comments.
Fortran/gfortran/CMakeLists.txt
265

Right, we can keep the empty files, and reset the lists here instead of in every file.
This is just a suggestion. Feel free to merge it as-is.

tblah updated this revision to Diff 544666.Jul 27 2023, 2:36 AM

Changes:

  • Reset variables in CMakeLists.txt so we don't need to do it in every included CMake file
  • Add support for allow lists in EnabledFilesFEATURE.cmake, using the list provided by @vzakhari
tblah edited the summary of this revision. (Show Details)Jul 27 2023, 2:37 AM

Have you created an EnabledFiles[FEATURE].cmake because the tests that are in DisabledFiles[FEATURE].cmake are also in one of the top-level deny lists? If so, do we need both accept and deny lists or do we just need an allow list here?

Do you have cases where a test passes without [FEATURE] but fails with [FEATURE]?

tblah added a comment.Aug 1 2023, 4:32 AM

Have you created an EnabledFiles[FEATURE].cmake because the tests that are in DisabledFiles[FEATURE].cmake are also in one of the top-level deny lists? If so, do we need both accept and deny lists or do we just need an allow list here?

Do you have cases where a test passes without [FEATURE] but fails with [FEATURE]?

I have created EnabledFilesFEATURE.cmake. We could merge those with DisabledFilesFEATURE if you prefer, I was worried it would be confusing to enable files in DisabledFilesFEATURE.cmake. Yes there are cases where tests pass without HLFIR but fail with it enabled. See the tests disabled by DisabledFilesHLFIR.cmake.

The lists might not be 100% accurate because there are new fixes landing all of the time, but I try to keep them up to date with things I review and fix myself. Once they are upstream, everyone can contribute to the accuracy of these lists.

We could merge those with DisabledFilesFEATURE if you prefer, I was worried it would be confusing to enable files in DisabledFilesFEATURE.cmake.

No, they should be left separate. I was just wondering if both were really necessary.

LGTM

vzakhari accepted this revision.Aug 1 2023, 8:23 AM

Thank you!