Fix -mno-altivec cannot overwrite -maltivec option

[kuang_he]

Summary

1. When -maltivec and -mno-altivec are present at the same time, altivec is still on instead of turned off.
2. When -fno-zvector and -fzvector are present at the same time, altivec is still on for s390x architecture.

[sfertile]

lib/Driver/Tools.cpp

4210–4216

If -fno-zvector has the same problem we should fix it as well.

[kuang_he]

Add fix and update test case for -fzvector option.

[uweigand]

SystemZ parts LGTM. Thanks!

[hfinkel]

LGTM

[nemanjai]

This seems to change the relationship between -m[no-]altivec and -f[no-]altivec which has been kind of contentious for a while. Can you add a note as to whether the new behaviour matches the GCC behaviour. Also, perhaps @echristo might want to chime in as to whether this is the desired behaviour or not.

[hfinkel]

In D30415#700534, @nemanjai wrote:

This seems to change the relationship between -m[no-]altivec and -f[no-]altivec which has been kind of contentious for a while. Can you add a note as to whether the new behaviour matches the GCC behaviour. Also, perhaps @echristo might want to chime in as to whether this is the desired behaviour or not.

Why do you say this? It only seems to affect whether the syntax extensions can be disabled via the appropriate flag.

[echristo]

Different suggestion:

Remove the faltivec option. Even gcc doesn't support it anymore afaict.

(Go ahead and commit the zvector part if you'd like).

-eric

[hfinkel]

In D30415#703398, @echristo wrote:

Different suggestion:

Remove the faltivec option. Even gcc doesn't support it anymore afaict.

What are you suggesting? Always having the language extensions on? Or explicitly tying the language extensions to the underlying target feature?

(Go ahead and commit the zvector part if you'd like).

-eric

[uweigand]

In D30415#703442, @hfinkel wrote:

In D30415#703398, @echristo wrote:

Different suggestion:

Remove the faltivec option. Even gcc doesn't support it anymore afaict.

What are you suggesting? Always having the language extensions on? Or explicitly tying the language extensions to the underlying target feature?

I'm a bit confused by this discussion. -faltivec and -maltivec are simply aliases, they do exactly the same thing; the clang-internal variable OPT_faltivec indicates the use of either -faltivec or -maltivec.

Is the suggestion to remove that flag completely, i.e. both -maltivec and -faltivec? This seems strange to me since -maltivec is used in many Makefiles etc. that would break if clang suddenly refused to accept the option.

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec as-is? I'd be less opposed to this since it probably breaks fewer users ... but I'm still not quite sure what it actually buys us. And in any case the patch currently under discussion here would still be necessary then, to fix -maltivec -mno-altivec ...

[echristo]

In D30415#703652, @uweigand wrote:

In D30415#703442, @hfinkel wrote:

In D30415#703398, @echristo wrote:

Different suggestion:

Remove the faltivec option. Even gcc doesn't support it anymore afaict.

What are you suggesting? Always having the language extensions on? Or explicitly tying the language extensions to the underlying target feature?

I'm a bit confused by this discussion. -faltivec and -maltivec are simply aliases, they do exactly the same thing; the clang-internal variable OPT_faltivec indicates the use of either -faltivec or -maltivec.

They didn't used to, I arranged it so that they did (technically breaking gcc compatibility) a while ago.

Is the suggestion to remove that flag completely, i.e. both -maltivec and -faltivec? This seems strange to me since -maltivec is used in many Makefiles etc. that would break if clang suddenly refused to accept the option.

No, just faltivec.

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec as-is? I'd be less opposed to this since it probably breaks fewer users ... but I'm still not quite sure what it actually buys us. And in any case the patch currently under discussion here would still be necessary then, to fix -maltivec -mno-altivec ...

No, remove faltivec and move forward with -maltivec/-mno-altivec but you should be able to remove a lot of the special handling at that point.

[uweigand]

In D30415#704761, @echristo wrote:

In D30415#703652, @uweigand wrote:

I'm a bit confused by this discussion. -faltivec and -maltivec are simply aliases, they do exactly the same thing; the clang-internal variable OPT_faltivec indicates the use of either -faltivec or -maltivec.

They didn't used to, I arranged it so that they did (technically breaking gcc compatibility) a while ago.

Well, mainline GCC doesn't have -faltivec at all and never had, I think this was only an Apple GCC extension ... Not sure what exactly the semantics of that was.

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec as-is? I'd be less opposed to this since it probably breaks fewer users ... but I'm still not quite sure what it actually buys us. And in any case the patch currently under discussion here would still be necessary then, to fix -maltivec -mno-altivec ...

No, remove faltivec and move forward with -maltivec/-mno-altivec but you should be able to remove a lot of the special handling at that point.

I'm still confused as to what exactly you're refering to here. As far as I can see, every single thing triggered by -faltivec / -maltivec in the compiler frontend would still be needed exactly the same if we only supported the -maltivec option name. So the only thing we'd save is literally the two lines in include/clang/Driver/Options.td that set up the alias.

Do you have an example of the "special handling" to remove you're thinking of?

[echristo]

In D30415#705196, @uweigand wrote:

In D30415#704761, @echristo wrote:

In D30415#703652, @uweigand wrote:

I'm a bit confused by this discussion. -faltivec and -maltivec are simply aliases, they do exactly the same thing; the clang-internal variable OPT_faltivec indicates the use of either -faltivec or -maltivec.

They didn't used to, I arranged it so that they did (technically breaking gcc compatibility) a while ago.

Well, mainline GCC doesn't have -faltivec at all and never had, I think this was only an Apple GCC extension ... Not sure what exactly the semantics of that was.

Sure it does and has for years. Check out rs6000/darwin.h :)

FWIW: It turns on maltivec and adds a -include of altivec.h

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec as-is? I'd be less opposed to this since it probably breaks fewer users ... but I'm still not quite sure what it actually buys us. And in any case the patch currently under discussion here would still be necessary then, to fix -maltivec -mno-altivec ...

No, remove faltivec and move forward with -maltivec/-mno-altivec but you should be able to remove a lot of the special handling at that point.

I'm still confused as to what exactly you're refering to here. As far as I can see, every single thing triggered by -faltivec / -maltivec in the compiler frontend would still be needed exactly the same if we only supported the -maltivec option name. So the only thing we'd save is literally the two lines in include/clang/Driver/Options.td that set up the alias.

Do you have an example of the "special handling" to remove you're thinking of?

Nearly all of the code in lib/Driver/ToolChains/Clang.cpp and lib/Driver/ToolChains/Arch/PPC.cpp that deal with altivec. Simplifying the interface by getting rid of needing to check multiple options.

-eric

[echristo]

In D30415#705889, @echristo wrote:

In D30415#705196, @uweigand wrote:

In D30415#704761, @echristo wrote:

In D30415#703652, @uweigand wrote:

I'm a bit confused by this discussion. -faltivec and -maltivec are simply aliases, they do exactly the same thing; the clang-internal variable OPT_faltivec indicates the use of either -faltivec or -maltivec.

They didn't used to, I arranged it so that they did (technically breaking gcc compatibility) a while ago.

Well, mainline GCC doesn't have -faltivec at all and never had, I think this was only an Apple GCC extension ... Not sure what exactly the semantics of that was.

Sure it does and has for years. Check out rs6000/darwin.h :)

FWIW: It turns on maltivec and adds a -include of altivec.h

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec as-is? I'd be less opposed to this since it probably breaks fewer users ... but I'm still not quite sure what it actually buys us. And in any case the patch currently under discussion here would still be necessary then, to fix -maltivec -mno-altivec ...

No, remove faltivec and move forward with -maltivec/-mno-altivec but you should be able to remove a lot of the special handling at that point.

I'm still confused as to what exactly you're refering to here. As far as I can see, every single thing triggered by -faltivec / -maltivec in the compiler frontend would still be needed exactly the same if we only supported the -maltivec option name. So the only thing we'd save is literally the two lines in include/clang/Driver/Options.td that set up the alias.

Do you have an example of the "special handling" to remove you're thinking of?

Nearly all of the code in lib/Driver/ToolChains/Clang.cpp and lib/Driver/ToolChains/Arch/PPC.cpp that deal with altivec. Simplifying the interface by getting rid of needing to check multiple options.

I have a patch to do this now. I'll plan on committing it in a bit.

-eric

[sfertile]

I have a patch to do this now. I'll plan on committing it in a bit.

Is there a way to mark the -f form of the option as deprecated? We should warn and suggest users switch to the -maltivec option for a release to give people a chance to update their builds.

[uweigand]

In D30415#705889, @echristo wrote:

In D30415#705196, @uweigand wrote:

Well, mainline GCC doesn't have -faltivec at all and never had, I think this was only an Apple GCC extension ... Not sure what exactly the semantics of that was.

Sure it does and has for years. Check out rs6000/darwin.h :)

FWIW: It turns on maltivec and adds a -include of altivec.h

Huh, I wasn't aware of that feature on Darwin, thanks for pointing it out ...

Nearly all of the code in lib/Driver/ToolChains/Clang.cpp and lib/Driver/ToolChains/Arch/PPC.cpp that deal with altivec. Simplifying the interface by getting rid of needing to check multiple options.

But why would that code no longer be necessary for -maltivec? Well, I guess I'll wait for your patch ...

If we indeed get rid of -faltivec, I'm wondering whether it would also make sense to get rid of -fzvector. This is just an alias for -mzvector, and it isn't supported by GCC either. I added it only because Richard Smith specifically asked for it when I contributed the feature here:
https://reviews.llvm.org/D11001

This should be a -f flag, not a -m flag. (I think we only support -maltivec for GCC compatibility.)

[echristo]

In D30415#706370, @sfertile wrote:

I have a patch to do this now. I'll plan on committing it in a bit.

Is there a way to mark the -f form of the option as deprecated? We should warn and suggest users switch to the -maltivec option for a release to give people a chance to update their builds.

I've added something and committed all of my work here:

echristo@athyra ~/s/l/t/clang> git svn dcommit
Committing to https://llvm.org/svn/llvm-project/cfe/trunk ...
A test/Driver/unsupported-faltivec.c
M docs/ClangCommandLineReference.rst
M docs/LanguageExtensions.rst
M include/clang/Basic/DiagnosticDriverKinds.td
M include/clang/Driver/Options.td
M lib/Basic/Targets.cpp
M lib/Driver/ToolChains/Arch/PPC.cpp
M lib/Driver/ToolChains/Clang.cpp
M lib/Frontend/CompilerInstance.cpp
M lib/Frontend/CompilerInvocation.cpp
M test/CodeGen/altivec.c
M test/CodeGen/builtins-ppc-altivec.c
M test/CodeGen/builtins-ppc-crypto-disabled.c
M test/CodeGen/builtins-ppc-crypto.c
M test/CodeGen/builtins-ppc-error.c
M test/CodeGen/builtins-ppc-htm.c
M test/CodeGen/builtins-ppc-p8vector.c
M test/CodeGen/builtins-ppc-p9vector.c
M test/CodeGen/builtins-ppc-quadword.c
M test/CodeGen/builtins-ppc-vsx.c
M test/CodeGen/ppc64-align-struct.c
M test/CodeGen/ppc64-vector.c
M test/CodeGen/ppc64le-aggregates.c
M test/Driver/ppc-features.cpp
M test/Headers/altivec-header.c
M test/Headers/altivec-intrin.c
M test/Parser/altivec-csk-bool.c
M test/Parser/altivec.c
M test/Parser/cxx-altivec.cpp
M test/Parser/vsx.c
M test/Sema/altivec-init.c
M test/Sema/builtins-ppc.c
M test/SemaCXX/altivec.cpp
M test/SemaCXX/cxx-altivec.cpp
Committed r298449

[sfertile]

Thanks Eric.

Kuang, commit r298449 contained this fix so you can close the review now.

[kuang_he]

In D30415#703652, @uweigand wrote:

In D30415#703442, @hfinkel wrote:

In D30415#703398, @echristo wrote:

Different suggestion:

Remove the faltivec option. Even gcc doesn't support it anymore afaict.

What are you suggesting? Always having the language extensions on? Or explicitly tying the language extensions to the underlying target feature?

I'm a bit confused by this discussion. -faltivec and -maltivec are simply aliases, they do exactly the same thing; the clang-internal variable OPT_faltivec indicates the use of either -faltivec or -maltivec.

Is the suggestion to remove that flag completely, i.e. both -maltivec and -faltivec? This seems strange to me since -maltivec is used in many Makefiles etc. that would break if clang suddenly refused to accept the option.

Or is the suggestion to simply remove the alias -faltivec, and leave -maltivec as-is? I'd be less opposed to this since it probably breaks fewer users ... but I'm still not quite sure what it actually buys us. And in any case the patch currently under discussion here would still be necessary then, to fix -maltivec -mno-altivec ...

lib/Driver/Tools.cpp

   // Emulated TLS is enabled by default on Android, and can be enabled manually
   // with -femulated-tls.
   bool EmulatedTLSDefault = Triple.isAndroid() || Triple.isWindowsCygwinEnvironment();
   if (Args.hasFlag(options::OPT_femulated_tls, options::OPT_fno_emulated_tls,
                    EmulatedTLSDefault))
     CmdArgs.push_back("-femulated-tls");
   // AltiVec-like language extensions aren't relevant for assembling.
   if (!isa<PreprocessJobAction>(JA) || Output.getType() != types::TY_PP_Asm) {
+    if (Args.hasFlag(options::OPT_faltivec,
+                     options::OPT_fno_altivec, /*Default=*/false))
       Args.AddLastArg(CmdArgs, options::OPT_faltivec);
+
+    if (Args.hasFlag(options::OPT_fzvector,
+                     options::OPT_fno_zvector, /*Default=*/false))
       Args.AddLastArg(CmdArgs, options::OPT_fzvector);
   }
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_show_template_tree);
   Args.AddLastArg(CmdArgs, options::OPT_fno_elide_type);
 
   // Forward flags for OpenMP. We don't do this if the current action is an
   // device offloading action other than OpenMP.
   if (Args.hasFlag(options::OPT_fopenmp, options::OPT_fopenmp_EQ,
                    options::OPT_fno_openmp, false) &&

test/Driver/ppc-features.cpp

 
 // CHECK: invalid argument '-faltivec' only allowed with 'ppc/ppc64/ppc64le'
 
 // Check that -fno-altivec and -mno-altivec correctly disable the altivec
 // target feature on powerpc.
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-1 %s
 // CHECK-1: "-target-feature" "-altivec"
+// CHECK-1-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -mno-altivec -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-2 %s
 // CHECK-2: "-target-feature" "-altivec"
+// CHECK-2-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -faltivec -mno-altivec -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-3 %s
 // CHECK-3: "-target-feature" "-altivec"
+// CHECK-3-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -maltivec -fno-altivec -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-4 %s
 // CHECK-4: "-target-feature" "-altivec"
+// CHECK-4-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -mno-altivec -faltivec -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-5 %s
-// CHECK-5-NOT: "-target-feature" "-altivec"
+// CHECK-5: "-target-feature" "+altivec"
+// CHECK-5: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -maltivec -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-6 %s
-// CHECK-6-NOT: "-target-feature" "-altivec"
+// CHECK-6: "-target-feature" "+altivec"
+// CHECK-6: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=7400 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-7 %s
 // CHECK-7: "-target-feature" "-altivec"
+// CHECK-7-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=g4 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-8 %s
 // CHECK-8: "-target-feature" "-altivec"
+// CHECK-8-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=7450 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-9 %s
 // CHECK-9: "-target-feature" "-altivec"
+// CHECK-9-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=g4+ -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-10 %s
 // CHECK-10: "-target-feature" "-altivec"
+// CHECK-10-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=970 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-11 %s
 // CHECK-11: "-target-feature" "-altivec"
+// CHECK-11-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=g5 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-12 %s
 // CHECK-12: "-target-feature" "-altivec"
+// CHECK-12-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=pwr6 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-13 %s
 // CHECK-13: "-target-feature" "-altivec"
+// CHECK-13-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=pwr7 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-14 %s
 // CHECK-14: "-target-feature" "-altivec"
+// CHECK-14-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=pwr8 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-15 %s
 // CHECK-15: "-target-feature" "-altivec"
+// CHECK-15-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -fno-altivec -mcpu=ppc64 -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK-16 %s
 // CHECK-16: "-target-feature" "-altivec"
+// CHECK-16-NOT: "-faltivec"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -mno-qpx -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOQPX %s
 // CHECK-NOQPX: "-target-feature" "-qpx"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -mno-qpx -mqpx -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-QPX %s
 // CHECK-QPX-NOT: "-target-feature" "-qpx"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu %s -mno-mfcrf -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOMFCRF %s

test/Driver/systemz-features.cpp

 // RUN: %clang -target s390x-unknown-linux-gnu %s -mno-vx -mvx -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-VX %s
 // CHECK-VX: "-target-feature" "+vector"
 // CHECK-VX-NOT: "-target-feature" "-vector"
 //
 // RUN: %clang -target s390x-unknown-linux-gnu %s -mno-vx -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOVX %s
 // RUN: %clang -target s390x-unknown-linux-gnu %s -mvx -mno-vx -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOVX %s
 // CHECK-NOVX: "-target-feature" "-vector"
 // CHECK-NOVX-NOT: "-target-feature" "+vector"
+//
+// RUN: %clang -target s390x-unknown-linux-gnu %s -fzvector -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-ZVECTOR %s
+// CHECK-ZVECTOR: "-fzvector"
+//
+// RUN: %clang -target s390x-unknown-linux-gnu %s -fzvector -fno-zvector -### -o %t.o 2>&1 | FileCheck -check-prefix=CHECK-NO-ZVECTOR %s
+// CHECK-NO-ZVECTOR-NOT: "-fzvector"