This is an archive of the discontinued LLVM Phabricator instance.

Fix -mno-altivec cannot overwrite -maltivec option
AbandonedPublic

Authored by kuang_he on Feb 27 2017, 10:09 AM.

Details

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.

Diff Detail

Event Timeline

kuang_he created this revision.Feb 27 2017, 10:09 AM
sfertile added inline comments.Feb 27 2017, 1:24 PM
lib/Driver/Tools.cpp
5436

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

kuang_he updated this revision to Diff 90927.Mar 7 2017, 1:45 PM
kuang_he edited the summary of this revision. (Show Details)

Add fix and update test case for -fzvector option.

uweigand edited edge metadata.Mar 14 2017, 7:13 AM

SystemZ parts LGTM. Thanks!

hfinkel accepted this revision.Mar 14 2017, 7:20 AM

LGTM

This revision is now accepted and ready to land.Mar 14 2017, 7:20 AM
nemanjai edited edge metadata.Mar 14 2017, 7:40 AM
nemanjai added a subscriber: echristo.

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.

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 requested changes to this revision.Mar 16 2017, 4:45 PM

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

This revision now requires changes to proceed.Mar 16 2017, 4:45 PM

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

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 ...

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.

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?

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

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 edited edge metadata.Mar 21 2017, 7:54 AM

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.

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.)

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

Thanks Eric.

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

kuang_he abandoned this revision.Mar 27 2017, 12:14 PM

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 ...