This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Add ClangFlags::TargetSpecific to simplify err_drv_unsupported_opt_for_target processing
ClosedPublic

Authored by MaskRay on May 26 2023, 2:04 PM.

Details

Summary

clang/lib/Driver/ToolChains/Clang.cpp has a lot of fragments like the following:

if (const Arg *A = Args.getLastArg(...)) {
  if (Triple is xxx)
    A->render(Args, CmdArgs);
  else
    D.Diag(diag::err_drv_unsupported_opt_for_target) << ...;
}

The problem is more apparent with a recent surge of AIX-specific options.

Introduce the TargetSpecific flag so that we can move the target-specific
options to ToolChains/*.cpp and ToolChains/Arch/*.cpp and overload the
warn_drv_unused_argument mechanism to give an err_drv_unsupported_opt_for_target
error.

Migrate -march=/-mcpu= and some AIX-specific options to use this simplified pattern.

Diff Detail

Event Timeline

MaskRay created this revision.May 26 2023, 2:04 PM
MaskRay requested review of this revision.May 26 2023, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 2:04 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the patch!

Recently we added the options mxcoff-roptr and mno-xcoff-roptr https://reviews.llvm.org/D144190 that fall into this the scope of this patch (e.g. https://github.com/llvm/llvm-project/blob/0508ac32cfcc1a9fc5a81d16de8d418dc5a0666b/clang/lib/Driver/ToolChains/Clang.cpp#LL5278C19-L5278C19), since they are only supported on AIX. Could you add the handling of options mxcoff-roptr and mno-xcoff-roptr?

Thanks for the patch!

Recently we added the options mxcoff-roptr and mno-xcoff-roptr https://reviews.llvm.org/D144190 that fall into this the scope of this patch (e.g. https://github.com/llvm/llvm-project/blob/0508ac32cfcc1a9fc5a81d16de8d418dc5a0666b/clang/lib/Driver/ToolChains/Clang.cpp#LL5278C19-L5278C19), since they are only supported on AIX. Could you add the handling of options mxcoff-roptr and mno-xcoff-roptr?

Hi @qiongsiwu1, thanks for chiming in. I am aware of -mxcoff-roptr. This patch picks some example patterns, but does not attempt to be comprehensive (on the GNU side, these are many many -m options that are not covered. E.g. LoongArch has a -mabi hack, so I cannot make -mabi TargetSpecific yet)
I can fold the -mxcoff-roptr change into this patch since it's straightforward.

MaskRay updated this revision to Diff 526614.May 30 2023, 7:45 AM

Handle -mxcoff-roptr as well

jansvoboda11 accepted this revision.May 30 2023, 9:37 AM

Nice improvement! Some tests seem to be failing due to the changes, but once those are fixed, LGTM.

This revision is now accepted and ready to land.May 30 2023, 9:37 AM
MaskRay updated this revision to Diff 526722.May 30 2023, 11:18 AM

Rebase.
Fix amdgpu. I suspect D145579 introduced latent -march= and -mcpu= interaction issues but I will leave the test coverage to amdgpu folks.

This revision was landed with ongoing or failed builds.May 30 2023, 11:21 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added a comment.EditedJun 13 2023, 12:59 PM

@jansvoboda11

For ELF operating systems (that traditionally use GCC), I think unsupported -m* options as errors are very desired as that's the GCC behavior and some configure system will prefer it. (For instance, they need to pass -Werror -mxxx to test availability, instead of just -mxxx.)

I think x86 -m* options are the most possible options to be misused.

% clang -c --target=aarch64-unknown-linux-gnu -msse4.2 a.c    # warning instead of error, not ideal
clang: warning: argument unused during compilation: '-msse4.2' [-Wunused-command-line-argument]
% aarch64-linux-gnu-gcc -msse4.2 a.c
aarch64-linux-gnu-gcc: error: unrecognized command-line option ‘-msse4.2’

For macOS universal binary builds (-arch), I wonder whether we want to refine the current behavior:

% clang -fdriver-only -c --target=x86_64-apple-darwin -arch arm64 -arch x86_64 -mavx512vl a.c   # no warning
% clang -fdriver-only -c --target=x86_64-apple-darwin -arch arm64 -mavx512vl a.c   # no -arch x86_64
clang: warning: argument unused during compilation: '-mavx512vl' [-Wunused-command-line-argument]     # or an error if we make mavx512vl TargetSpecific

The -arch arm64 -mavx512vl situation isn't very common. Though not-carefully-written CMake may make the mistakes: llvm/lib/Support/BLAKE3/CMakeLists.txt (fixed by f231829304d15dca0dad8bafe35c4277904db602) and in-tree misuse: tsan (https://github.com/llvm/llvm-project/issues/63270).
(I think the two use sites are unusual.)

dewen added a subscriber: dewen.Jul 28 2023, 8:15 PM