This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Update the visibility of Clang options in Flang
ClosedPublic

Authored by awarzynski on Aug 14 2023, 12:40 AM.

Details

Summary

Prior to D157151, there was no mechanism to "disable" unsupported Clang
options in Flang. While the "help" text (flang-new -help) was indeed
configured not to display such options, the Flang compiler driver would
happily consume such options and only issue a warning when there was no
logic to parse it:

flang-new -fno-experimental-relative-c++-abi-vtables file.f90
flang-new: warning: argument unused during compilation: '-fno-experimental-relative-c++-abi-vtables'

D157151 introduces visibility flags. In particular, all Clang options
gain a visibility flag (Default) that can be excluded when in Flang
driver mode. This way the above invocation will lead to:

flang-new: error: unknown argument '-fno-experimental-relative-c++-abi-vtables'; did you mean '-Xclang -fno-experimental-relative-c++-abi-vtables'?

This won't work unless all options/flags supported by Flang have their
visibility flags updated accordingly, hence the changes in Options.td.
Moving forward, this change will allow Flang better control over the
supported options.

Depends on D157151

Diff Detail

Event Timeline

awarzynski created this revision.Aug 14 2023, 12:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Aug 14 2023, 12:40 AM
Herald added a project: Restricted Project. · View Herald Transcript
tblah added inline comments.Aug 14 2023, 2:40 AM
clang/lib/Driver/Driver.cpp
6497–6498

I presume this TODO can go now?

flang/test/Driver/target-cpu-features.f90
14 ↗(On Diff #549810)

Why doesn't this fail now?

I can't speak to which flags should be present in flang-new or not, but in principle this looks great. You'll need to update the patch to use the "ClangOption" spelling rather than "Default" of course, as discussed in https://reviews.llvm.org/D157151

clang/lib/Driver/Driver.cpp
6497–6498

We can remove the TODO comment with this change :)

This looks good to me (minus the issue @tblah mentioned), while this will mean that we also error on gfortran options we don't recognize, I don't personally think that's a problem as in my personal experience Fortran users are used to different compilers accepting different options and dealing with that.

awarzynski marked 2 inline comments as done.Aug 14 2023, 12:05 PM

Thank you all for reviewing!

I can't speak to which flags should be present in flang-new or not

That's determined by what's tested/used in tests.

You'll need to update the patch to use the "ClangOption" spelling rather than "Default" of course, as discussed in https://reviews.llvm.org/D157151

Yup, no problem. I may wait for your changes to land first.

flang/test/Driver/target-cpu-features.f90
14 ↗(On Diff #549810)

Missing TargetSpecific flag in the definition of the mcpu_EQ option. Sorry i didn't have time to debug earlier. To be fixed in the next revision.

Add missing TargetSpecific flag to the definition of mcpu_EQ, remove redundant TODO

bogner accepted this revision.Aug 14 2023, 12:44 PM
This revision is now accepted and ready to land.Aug 14 2023, 12:44 PM
tblah accepted this revision.Aug 15 2023, 2:24 AM

Looks good to me, thanks for this!

Rebase on top of main

MaskRay accepted this revision.Aug 16 2023, 4:11 PM
This revision was landed with ongoing or failed builds.Aug 17 2023, 12:13 PM
This revision was automatically updated to reflect the committed changes.