[Clang] Make offloading flags accept '-' and '--'

[jhuber6]

Summary

Currently all of the flags beginning with --offload such as
--offload-arch or --offload-device-only require the double-dash
form. However, if a user uses a single dash it will instead name a file
'ffload-arch' for example. This has been the cause of a lot of user
confusion. This patch changes these options to also accept a single
dash. This is similar to many other driver arguments beginning with
-o.

[JonChesterfield]

Like that a lot, good quality of life improvement.

[MaskRay]

Single-dash long options starting with -o conflict with the short option -o so I am unsure I like this direction.

However, if a user uses a single dash it will instead name a file 'ffload-arch' for example.

That's intended. A user should be able to specify an output name ffload-arch with -offload-arch.

This is similar to many other driver arguments beginning with -o.

What options?

[MaskRay]

I think this should be reverted. It's moving in the backward direction.

[jhuber6]

In D135076#3830914, @MaskRay wrote:

Single-dash long options starting with -o conflict with the short option -o so I am unsure I like this direction.

However, if a user uses a single dash it will instead name a file 'ffload-arch' for example.

That's intended. A user should be able to specify an output name ffload-arch with -offload-arch.

I made this patch primarily because I've gotten tired of getting messages from users wondering why their code wasn't offloading to the correct architecture only when they used -offload-arch=. This is something that's not clearly obvious to users and I think it's highly unlikely that a user would want a file named -ffload-arch.

This is similar to many other driver arguments beginning with -o.

What options?

There were just a few I saw after running clang --help 2>&1 | grep -e '^\ *-o. It was just a handful but I figured having anything more than -o meant this would be uncontroversial.

In D135076#3830931, @MaskRay wrote:

I think this should be reverted. It's moving in the backward direction.

That's fine if you think that this is unacceptable with the existing semantics. Maybe an alternative route would be to re-parse -o options and give a warning if it matches a recognized option? I feel like using -o without a space is uncommon enough to warrant it.

[jdoerfert]

In D135076#3830914, @MaskRay wrote:

Single-dash long options starting with -o conflict with the short option -o so I am unsure I like this direction.

However, if a user uses a single dash it will instead name a file 'ffload-arch' for example.

That's intended. A user should be able to specify an output name ffload-arch with -offload-arch.

This is similar to many other driver arguments beginning with -o.

What options?

./bin/clang --help-hidden | grep "  \-o"
  -objcmt-allowlist-dir-path=<value>
  -objcmt-atomic-property Make migration to 'atomic' properties
  -objcmt-migrate-all     Enable migration to modern ObjC
  -objcmt-migrate-annotation
  -objcmt-migrate-designated-init
  -objcmt-migrate-instancetype
  -objcmt-migrate-literals
  -objcmt-migrate-ns-macros
  -objcmt-migrate-property-dot-syntax
  -objcmt-migrate-property
  -objcmt-migrate-protocol-conformance
  -objcmt-migrate-readonly-property
  -objcmt-migrate-readwrite-property
  -objcmt-migrate-subscripting
  -objcmt-ns-nonatomic-iosonly
  -objcmt-returns-innerpointer-property
  -objcmt-whitelist-dir-path=<value>
  -object-file-name=<file>
  -o <file>               Write output to <file>

The alternative is to at least warn if users write -offload... rather than --offlaod...

[MaskRay]

There are traditionally some single-dash long options (perhaps classical Mac OS style) which conflict with short options. I think nowadays we try to avoid such single-dash short options.
(For example, GNU ld now requires all new long options to use two-dashes, after knowing the problem with -omagic.)

I can make a patch to make cc1 -o Separate instead of JoinedOrSeparate, which will hopefully solve your pain of specifying a misspelled -offloat* cc1 option without good diagnostics.

[jhuber6]

In D135076#3830972, @MaskRay wrote:

There are traditionally some single-dash long options (perhaps classical Mac OS style) which conflict with short options. I think nowadays we try to avoid such single-dash short options.
(For example, GNU ld now requires all new long options to use two-dashes, after knowing the problem with -omagic.)

I can make a patch to make cc1 -o Separate instead of JoinedOrSeparate, which will hopefully solve your pain of specifying a misspelled -offloat* cc1 option without good diagnostics.

If that's possible, then that would be a good solution. I figured that allowing -ofoo was set in stone and this was the less controversial option. Just for reference, the problem I commonly encounter with users is the following situation

clang foo.cu -offload-arch=sm_80 -o foo // No offload arch, the  first -o isn't observed as its replaced by the second one.

[MaskRay]

In D135076#3830989, @jhuber6 wrote:

In D135076#3830972, @MaskRay wrote:

There are traditionally some single-dash long options (perhaps classical Mac OS style) which conflict with short options. I think nowadays we try to avoid such single-dash short options.
(For example, GNU ld now requires all new long options to use two-dashes, after knowing the problem with -omagic.)

I can make a patch to make cc1 -o Separate instead of JoinedOrSeparate, which will hopefully solve your pain of specifying a misspelled -offloat* cc1 option without good diagnostics.

If that's possible, then that would be a good solution. I figured that allowing -ofoo was set in stone and this was the less controversial option. Just for reference, the problem I commonly encounter with users is the following situation

A cc1 specific Separate -o only helps misspelled -offload-*...

clang foo.cu -offload-arch=sm_80 -o foo // No offload arch, the  first -o isn't observed as its replaced by the second one.

I think these users have to accept that -offload-arch=sm_80 means -o ffload-arch=sm_80.
There are a number of unfortunate Separate short options. -o is among them the most used. Long options have to avoid conflicts with -o by not having the single-dash form.

[MaskRay]

We really want these --offload-* users to stick with one canonical form, not -offload-* in some places while --offload-* in other places.

Another angle is that people find -offload-* working with a new clang may try -offload-* on an old clang and get -o ffload-*.

And -offload-* doesn't help misspelled options.

[jhuber6]

In D135076#3831298, @MaskRay wrote:

We really want these --offload-* users to stick with one canonical form, not -offload-* in some places while --offload-* in other places.

Another angle is that people find -offload-* working with a new clang may try -offload-* on an old clang and get -o ffload-*.

And -offload-* doesn't help misspelled options.

This is fine, as long as users get more distinct feedback that -offload-arch isn't doing what they think it does. Normally we'd get some error and a helpful suggestion if the option is misspelled, but with -o options we don't get anything. My only qualm with the current state is that it's not obvious that -offload-arch is actually -o ffload-arch for most cases.

[MaskRay]

In D135076#3831307, @jhuber6 wrote:

In D135076#3831298, @MaskRay wrote:

We really want these --offload-* users to stick with one canonical form, not -offload-* in some places while --offload-* in other places.

Another angle is that people find -offload-* working with a new clang may try -offload-* on an old clang and get -o ffload-*.

And -offload-* doesn't help misspelled options.

This is fine, as long as users get more distinct feedback that -offload-arch isn't doing what they think it does. Normally we'd get some error and a helpful suggestion if the option is misspelled, but with -o options we don't get anything.
My only qualm with the current state is that it's not obvious that -offload-arch is actually -o ffload-arch for most cases.

You can make -oxxx an error if offloading is used (-o xxx is still allowed). Then no --offload-* needs a -offload-* form. They are new users and they can likely live with stricter rules.
It's perhaps too destructive to remove -oxxx for regular targets.

This patch probably provides some convenience but regresses other aspects, and I think it should be reverted.

[jhuber6]

In D135076#3831340, @MaskRay wrote:

In D135076#3831307, @jhuber6 wrote:

In D135076#3831298, @MaskRay wrote:

We really want these --offload-* users to stick with one canonical form, not -offload-* in some places while --offload-* in other places.

Another angle is that people find -offload-* working with a new clang may try -offload-* on an old clang and get -o ffload-*.

And -offload-* doesn't help misspelled options.

This is fine, as long as users get more distinct feedback that -offload-arch isn't doing what they think it does. Normally we'd get some error and a helpful suggestion if the option is misspelled, but with -o options we don't get anything.
My only qualm with the current state is that it's not obvious that -offload-arch is actually -o ffload-arch for most cases.

You can make -oxxx an error if offloading is used (-o xxx is still allowed). Then no --offload-* needs a -offload-* form.

The problem here is that the --offload-* options are used to enable the offloading toolchain for some targets (e.g. OpenMP). Would a check on -o' that emits a warning if it matches closely a known option work? E.g. -offload-arch` would warn that the user may mean --offload-arch.

This patch probably provides some convenience but regresses other aspects, and I think it should be reverted.

If you think this should be reverted then I'm fine with it. I would like to see some kind of solution to this problem however, as I have dealt with this problem many times when working with users.

[MaskRay]

In D135076#3831347, @jhuber6 wrote:

In D135076#3831340, @MaskRay wrote:

In D135076#3831307, @jhuber6 wrote:

In D135076#3831298, @MaskRay wrote:

We really want these --offload-* users to stick with one canonical form, not -offload-* in some places while --offload-* in other places.

Another angle is that people find -offload-* working with a new clang may try -offload-* on an old clang and get -o ffload-*.

And -offload-* doesn't help misspelled options.

This is fine, as long as users get more distinct feedback that -offload-arch isn't doing what they think it does. Normally we'd get some error and a helpful suggestion if the option is misspelled, but with -o options we don't get anything.
My only qualm with the current state is that it's not obvious that -offload-arch is actually -o ffload-arch for most cases.

You can make -oxxx an error if offloading is used (-o xxx is still allowed). Then no --offload-* needs a -offload-* form.

The problem here is that the --offload-* options are used to enable the offloading toolchain for some targets (e.g. OpenMP). Would a check on -o' that emits a warning if it matches closely a known option work? E.g. -offload-arch` would warn that the user may mean --offload-arch.

If we don't have Joined -o, we won't have the option collision problem. But we have Joined -o, and we should not introduce new aliases to collide with -o.
I am fine if Joined -o can go away but there are too many uses so such a change would be destructive.

My idea is to just disallow Joined -o when targeting a specific environment (e.g. when offloading toolchain is used).

This patch probably provides some convenience but regresses other aspects, and I think it should be reverted.

If you think this should be reverted then I'm fine with it. I would like to see some kind of solution to this problem however, as I have dealt with this problem many times when working with users.

As mentioned, making users stick with more forms instead of all using the canonical form will do harm in other aspects.
I have explained these aspects in my previous comment.

[jhuber6]

In D135076#3831363, @MaskRay wrote:

My idea is to just disallow Joined -o when targeting a specific environment (e.g. when offloading toolchain is used).

This seems difficult as we only know which offloading toolchains are active after we fully construct the Compilation in the driver. We could potentially re-parse some options afterwards to do some static checks like this.

[MaskRay]

In D135076#3831371, @jhuber6 wrote:

In D135076#3831363, @MaskRay wrote:

My idea is to just disallow Joined -o when targeting a specific environment (e.g. when offloading toolchain is used).

This seems difficult as we only know which offloading toolchains are active after we fully construct the Compilation in the driver. We could potentially re-parse some options afterwards to do some static checks like this.

I think I'm fine if you want to warning for every Joined -o use of ffloading-*. Created D135093 to revert this patch.

[MaskRay]

Another idea is to reject multiple -o if some are used as the Joined form. Note: multiple Separate -o should be allowed.
It will not catch -offload-* when -o output is not specified, but is probably useful enough.

[MaskRay]

Sorry, but I'll revert this patch shortly.

[tra]

In D135076#3831563, @MaskRay wrote:

Another idea is to reject multiple -o if some are used as the Joined form. Note: multiple Separate -o should be allowed.
It will not catch -offload-* when -o output is not specified, but is probably useful enough.

Is there a reason we should not always reject -o<existing double-dash option>? I doubt there are many real life uses of such output names and if/when someone needs them, it's easy enough to disambiguate by using a separate -o argument or -o./output.
This would address the issue for all --o* options.

clang/include/clang/Driver/Options.td

 def Xopenmp_target : Separate<["-"], "Xopenmp-target">, Group<CompileOnly_Group>,
   HelpText<"Pass <arg> to the target offloading toolchain.">, MetaVarName<"<arg>">;
 def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">, Group<CompileOnly_Group>,
   HelpText<"Pass <arg> to the target offloading toolchain identified by <triple>.">,
   MetaVarName<"<triple> <arg>">;
 def z : Separate<["-"], "z">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass -z <arg> to the linker">, MetaVarName<"<arg>">,
   Group<Link_Group>;
-def offload_link : Flag<["--"], "offload-link">, Group<Link_Group>,
+def offload_link : Flag<["--", "-"], "offload-link">, Group<Link_Group>,
   HelpText<"Use the new offloading linker to perform the link job.">;
 def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass <arg> to the linker">, MetaVarName<"<arg>">,
   Group<Link_Group>;
 def Xoffload_linker : JoinedAndSeparate<["-"], "Xoffload-linker">,
   HelpText<"Pass <arg> to the offload linkers or the ones idenfied by -<triple>">,
   MetaVarName<"<triple> <arg>">, Group<Link_Group>;
 def Xpreprocessor : Separate<["-"], "Xpreprocessor">, Group<Preprocessor_Group>,

 
 def gpu_use_aux_triple_only : Flag<["--"], "gpu-use-aux-triple-only">,
   InternalDriverOpt, HelpText<"Prepare '-aux-triple' only without populating "
                               "'-aux-target-cpu' and '-aux-target-feature'.">;
 def cuda_include_ptx_EQ : Joined<["--"], "cuda-include-ptx=">, Flags<[NoXarchOption]>,
   HelpText<"Include PTX for the following GPU architecture (e.g. sm_35) or 'all'. May be specified more than once.">;
 def no_cuda_include_ptx_EQ : Joined<["--"], "no-cuda-include-ptx=">, Flags<[NoXarchOption]>,
   HelpText<"Do not include PTX for the following GPU architecture (e.g. sm_35) or 'all'. May be specified more than once.">;
-def offload_arch_EQ : Joined<["--"], "offload-arch=">, Flags<[NoXarchOption]>,
+def offload_arch_EQ : Joined<["--", "-"], "offload-arch=">, Flags<[NoXarchOption]>,
   HelpText<"CUDA offloading device architecture (e.g. sm_35), or HIP offloading target ID in the form of a "
            "device architecture followed by target ID features delimited by a colon. Each target ID feature "
            "is a pre-defined string followed by a plus or minus sign (e.g. gfx908:xnack+:sramecc-).  May be "
            "specified more than once.">;
 def cuda_gpu_arch_EQ : Joined<["--"], "cuda-gpu-arch=">, Flags<[NoXarchOption]>,
   Alias<offload_arch_EQ>;
 def cuda_feature_EQ : Joined<["--"], "cuda-feature=">, HelpText<"Manually specify the CUDA feature to use">;
 def hip_link : Flag<["--"], "hip-link">,
   HelpText<"Link clang-offload-bundler bundles for HIP">;
 def no_hip_rt: Flag<["-"], "no-hip-rt">,
   HelpText<"Do not link against HIP runtime libraries">;
-def no_offload_arch_EQ : Joined<["--"], "no-offload-arch=">, Flags<[NoXarchOption]>,
+def no_offload_arch_EQ : Joined<["--", "-"], "no-offload-arch=">, Flags<[NoXarchOption]>,
   HelpText<"Remove CUDA/HIP offloading device architecture (e.g. sm_35, gfx906) from the list of devices to compile for. "
            "'all' resets the list to its default value.">;
 def emit_static_lib : Flag<["--"], "emit-static-lib">,
   HelpText<"Enable linker job to emit a static library.">;
 def no_cuda_gpu_arch_EQ : Joined<["--"], "no-cuda-gpu-arch=">, Flags<[NoXarchOption]>,
   Alias<no_offload_arch_EQ>;
 def cuda_noopt_device_debug : Flag<["--"], "cuda-noopt-device-debug">,
   HelpText<"Enable device-side debug info generation. Disables ptxas optimizations.">;

 defm autolink : BoolFOption<"autolink",
   CodeGenOpts<"Autolink">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "Disable generation of linker directives for automatic library linking">,
   PosFlag<SetTrue>>;
 
 // In the future this option will be supported by other offloading
 // languages and accept other values such as CPU/GPU architectures,
 // offload kinds and target aliases.
-def offload_EQ : CommaJoined<["--"], "offload=">, Flags<[NoXarchOption]>,
+def offload_EQ : CommaJoined<["--", "-"], "offload=">, Flags<[NoXarchOption]>,
   HelpText<"Specify comma-separated list of offloading target triples (CUDA and HIP only)">;
 
 // C++ Coroutines TS
 defm coroutines_ts : BoolFOption<"coroutines-ts",
   LangOpts<"Coroutines">, Default<cpp20.KeyPath>,
   PosFlag<SetTrue, [CC1Option], "Enable support for the C++ Coroutines TS">,
   NegFlag<SetFalse>>;
 
def fopenmp_target_new_runtime : Flag<["-"], "fopenmp-target-new-runtime">,
   Group<f_Group>, Flags<[CC1Option, HelpHidden]>;
 def fno_openmp_target_new_runtime : Flag<["-"], "fno-openmp-target-new-runtime">,
   Group<f_Group>, Flags<[CC1Option, HelpHidden]>;
 defm openmp_optimistic_collapse : BoolFOption<"openmp-optimistic-collapse",
   LangOpts<"OpenMPOptimisticCollapse">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option]>, NegFlag<SetFalse>, BothFlags<[NoArgumentUnused, HelpHidden]>>;
 def static_openmp: Flag<["-"], "static-openmp">,
   HelpText<"Use the static host OpenMP runtime while linking.">;
-def offload_new_driver : Flag<["--"], "offload-new-driver">, Flags<[CC1Option]>, Group<f_Group>,
+def offload_new_driver : Flag<["--", "-"], "offload-new-driver">, Flags<[CC1Option]>, Group<f_Group>,
   MarshallingInfoFlag<LangOpts<"OffloadingNewDriver">>, HelpText<"Use the new driver for offloading compilation.">;
-def no_offload_new_driver : Flag<["--"], "no-offload-new-driver">, Flags<[CC1Option]>, Group<f_Group>,
+def no_offload_new_driver : Flag<["--", "-"], "no-offload-new-driver">, Flags<[CC1Option]>, Group<f_Group>,
   HelpText<"Don't Use the new driver for offloading compilation.">;
-def offload_device_only : Flag<["--"], "offload-device-only">,
+def offload_device_only : Flag<["--", "-"], "offload-device-only">,
   HelpText<"Only compile for the offloading device.">;
-def offload_host_only : Flag<["--"], "offload-host-only">,
+def offload_host_only : Flag<["--", "-"], "offload-host-only">,
   HelpText<"Only compile for the offloading host.">;
-def offload_host_device : Flag<["--"], "offload-host-device">,
+def offload_host_device : Flag<["--", "-"], "offload-host-device">,
   HelpText<"Only compile for the offloading host.">;
 def cuda_device_only : Flag<["--"], "cuda-device-only">, Alias<offload_device_only>,
   HelpText<"Compile CUDA code for device only">;
 def cuda_host_only : Flag<["--"], "cuda-host-only">, Alias<offload_host_only>,
   HelpText<"Compile CUDA code for host only. Has no effect on non-CUDA compilations.">;
 def cuda_compile_host_device : Flag<["--"], "cuda-compile-host-device">, Alias<offload_host_device>,
   HelpText<"Compile CUDA code for both host and device (default). Has no "
            "effect on non-CUDA compilations.">;