This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP][Lower] Fail compilation with TODO errors for unsupported clauses
ClosedPublic

Authored by skatrak on Aug 16 2023, 6:23 AM.

Details

Summary

This patch explicitly marks supported clauses for OpenMP directives missing an
implementation, so that compilation fails if they are specified, rather than
silently ignoring them.

Diff Detail

Event Timeline

skatrak created this revision.Aug 16 2023, 6:23 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 16 2023, 6:23 AM
skatrak requested review of this revision.Aug 16 2023, 6:23 AM
skatrak updated this revision to Diff 550733.Aug 16 2023, 6:43 AM

Simplify code searching for multiple types of clauses.

flang/lib/Lower/OpenMP.cpp
1791

Can the clause names be also printed?

skatrak updated this revision to Diff 551481.Aug 18 2023, 6:00 AM
skatrak marked an inline comment as done.

Print clause name in TODO error.

LG. Check with the gfortran testsuite before submitting.

I generally use the following instructions for running the testsuite.

git clone https://github.com/llvm/llvm-test-suite.git
cd llvm-test-suite
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm-project/build_release/bin/clang  -DCMAKE_CXX_COMPILER=$HOME/llvm-project/build_release/bin/clang++ -DCMAKE_Fortran_COMPILER=$HOME/llvm-project/build_release/bin/flang-new -DTEST_SUITE_FORTRAN=On -DTEST_SUITE_SUBDIRS=Fortran -DTEST_SUITE_FORTRAN_ISO_C_HEADER_DIR=$HOME/llvm-project/flang/include/flang ../
make -j48
NO_STOP_MESSAGE=1 $HOME/llvm-project/build_release/bin/llvm-lit -v  .
This revision is now accepted and ready to land.Aug 18 2023, 6:23 AM

LG. Check with the gfortran testsuite before submitting.

I generally use the following instructions for running the testsuite.

git clone https://github.com/llvm/llvm-test-suite.git
cd llvm-test-suite
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm-project/build_release/bin/clang  -DCMAKE_CXX_COMPILER=$HOME/llvm-project/build_release/bin/clang++ -DCMAKE_Fortran_COMPILER=$HOME/llvm-project/build_release/bin/flang-new -DTEST_SUITE_FORTRAN=On -DTEST_SUITE_SUBDIRS=Fortran -DTEST_SUITE_FORTRAN_ISO_C_HEADER_DIR=$HOME/llvm-project/flang/include/flang ../
make -j48
NO_STOP_MESSAGE=1 $HOME/llvm-project/build_release/bin/llvm-lit -v  .

Thank you for looking into this and for the detailed instructions. After rebasing I started having issues building due to "flang-new: error: unknown argument: '-w'" errors. Could this be related to D157837 maybe?

Before that, I was seeing the pr107214-6.f90 and pr107214-3.f90 tests failing as well (the ones that you disabled after the patch I landed broke them). Could they be failing now because they are expected to pass and this patch makes them fail again (for the wrong reasons, as before)?

Thank you for looking into this and for the detailed instructions. After rebasing I started having issues building due to "flang-new: error: unknown argument: '-w'" errors. Could this be related to D157837 maybe?

It is. Andrzej is looking into it.

Thank you for looking into this and for the detailed instructions. After rebasing I started having issues building due to "flang-new: error: unknown argument: '-w'" errors. Could this be related to D157837 maybe?

It is. Andrzej is looking into it.

Thank you, those tests started passing again. I just opened D158417 to modify the enabled/disabled gfortran tests impacted by this patch, though I'm not sure what the right procedure would be to get both of these working together at the same time.

If I understand correctly the goal is to land both changes without breaking builds, putting the llvm-test-suite change in first, then this is the way I'd do that.

The longest bot build time for Linaro's flang bots is around 1h10 minutes, so if you leave a gap >= that you'll get a build with just the test suite change, then with that and this change. It's unlikely any bot will only get this change only, since most of them just use llvm-test-suite from HEAD regardless of the llvm-project hash.

skatrak updated this revision to Diff 552285.Aug 22 2023, 3:01 AM

Rebase before landing.