Page MenuHomePhabricator

[FPEnv][Clang][Driver] Disable constrained floating point on targets lacking support.
ClosedPublic

Authored by kpn on Jun 1 2020, 1:56 PM.

Details

Summary

We currently have strict floating point/constrained floating point enabled for all targets. Constrained SDAG nodes get converted to the regular ones before reaching the target layer. In theory this should be fine.

However, the changes are exposed to users through multiple clang options already in use in the field, and the changes are _completely_ _untested_ on almost all of our targets. Bugs have already been found, like "https://bugs.llvm.org/show_bug.cgi?id=45274".

This patch disables constrained floating point options in clang everywhere except X86 and SystemZ. A warning will be printed when this happens.

Diff Detail

Event Timeline

kpn created this revision.Jun 1 2020, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 1:56 PM
kpn added a comment.Jun 1 2020, 1:59 PM

This currently triggers a failure in test "Clang :: Misc/warning-flags.c". Suggestions on how to fix that correctly are welcome.

Clang :: Misc/warning-flags.c

Like the file says, all warnings should be controlled by a -W flag. If you're going to add a new warning, you need a corresponding flag.

arsenm requested changes to this revision.Jun 1 2020, 3:11 PM
arsenm added a subscriber: arsenm.

If this chooses to just blindly ignore the setting, how is development supposed to be done on other targets? I'd prefer if this behaved more like -fglobal-isel, where it warns on unsupported targets but doesn't decide to ignore you

This revision now requires changes to proceed.Jun 1 2020, 3:11 PM

The problem for the command-line arguments in particular is that they aren't really new; clang has been eating them for a long time, without any warning. So if -frounding-math crashes the compiler, that's a regression.

arsenm added a comment.Jun 1 2020, 4:38 PM

The problem for the command-line arguments in particular is that they aren't really new; clang has been eating them for a long time, without any warning. So if -frounding-math crashes the compiler, that's a regression.

Could we get an -fexperimental-something flag to override this behavior then?

kpn added a comment.Jun 2 2020, 6:16 AM

The problem for the command-line arguments in particular is that they aren't really new; clang has been eating them for a long time, without any warning. So if -frounding-math crashes the compiler, that's a regression.

Could we get an -fexperimental-something flag to override this behavior then?

The vast majority of the work needed is in llvm and not clang. Most if not all testing is done with the llc executable. There is some work in clang's CGBuiltins.cpp, but that's pretty small compared to llvm. And with this patch the enabling of the clang support is only one line. That seems cheaper than yet another command line flag. Someone doing development of the clang parts is going to be building clang anyway so I don't see how a one line change is much of a burden.

The flags for strict fp and rounding are already pretty complicated already. I'd hate to make it even worse with yet another flag just to avoid having a developer have to carry one line in one source file. And when clang+llvm is ready for a target that one line would be needed _anyway_. So what would a new -fexperimental-something flag gain us?

Also, I'm not sure that the constrained intrinsics will ever be implemented on every last one of our targets. So a new flag would complicate matters basically forever.

The PowerPC backend is also adding the constraint float point support and almost done.

kpn updated this revision to Diff 270219.Jun 11 2020, 1:25 PM
kpn retitled this revision from [FPEnv][Clang][Driver][WIP] Disable constrained floating point on targets lacking support. to [FPEnv][Clang][Driver] Disable constrained floating point on targets lacking support..

Added new warnings to a new group "unsupported-floating-point-opt". The warning can be disabled. The disabling of constrained floating point is unaffected by the status of the warning.

I added PowerPC to the list of targets that are _not_ disabled by this patch since that target is close to parity with X86 and SystemZ.

Is there anything left?

wristow added inline comments.Jun 22 2020, 12:37 PM
clang/test/CodeGen/fp-strictfp.cpp
2

Need to remove the "tee" command in the pipe sequence.

kpn updated this revision to Diff 272530.Jun 22 2020, 1:04 PM

Remove debugging command left in accidentally. Rebase.

kpn added a comment.Jun 30 2020, 5:58 AM

Ping. I'm really hoping to get this into 11. Otherwise we're going multiple releases with options that people already use causing crashes on most architectures.

efriedma added subscribers: nikic, john.brawn.

I'm not sure what the status is of constrained fp on arm/aarch64; some patches went in, but I'm not sure what sort of testing was done. @john.brawn @nikic ?

Otherwise, I think this patch makes sense.

nemanjai accepted this revision.Jun 30 2020, 1:05 PM

As far as I'm concerned, this is fine for now. We can remove these once all in-tree target have implemented their support.
LGTM but maybe give a couple of days for others to chime in.

clang/lib/Basic/Targets/PPC.h
86 ↗(On Diff #272530)

I don't think we need this for now. Close is not quite there. @steven.zhang I would prefer that we initially turn this off and only flip it on once the support is complete.
Also, is the support that is currently under development for both 32 and 64 bit architectures? If it is 64 bit only, then we can enable it only there once it is done.

steven.zhang added inline comments.Jun 30 2020, 6:35 PM
clang/lib/Basic/Targets/PPC.h
86 ↗(On Diff #272530)

Yes, it hasn't been finished yet. Agree that we can turn on it later when it is done.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 6 2020, 10:33 AM
This revision was automatically updated to reflect the committed changes.

This seems to break tests everywhere, eg http://45.33.8.238/linux/22152/step_7.txt or
http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/11251

Please take a look and revert if the fix takes a while.

kpn added a comment.Jul 6 2020, 11:45 AM

Already on it. I hope I got it now.

MaskRay added a subscriber: MaskRay.EditedJul 6 2020, 11:46 AM

Note also that @arsenm is still a blocking reviewer. It is generally expected that all feedback is acknowledged. @kpn you should probably have waited for @arsenm to explicitly clear the blocker.

Note also that @arsenm is still a blocking reviewer. It is generally expected that all feedback is acknowledged. @kpn you should probably have waited for @arsenm to explicitly clear the blocker.

I think this is one of the finer points of phabricator usage that are generally ignored.

I'd still like to have a way to force this on unhandled targets, but I don't care that much about this

kpn reopened this revision.Jul 6 2020, 12:20 PM
bcain added a subscriber: bcain.Jul 7 2020, 10:35 AM
kpn updated this revision to Diff 276166.Jul 7 2020, 12:01 PM

Add the -fexperimental-strict-floating-point flag to enable on hosts that are not marked as supporting strict FP yet. Add test and documentation.

Update tests to use the new flag. This eliminates the XFAIL lines and should keep the tests running like before.

Hopefully this will work for 11.

nikic resigned from this revision.Jul 8 2020, 9:03 AM
sidneym added a subscriber: sidneym.Jul 8 2020, 1:25 PM
arsenm accepted this revision.Jul 9 2020, 2:44 PM
This revision is now accepted and ready to land.Jul 9 2020, 2:44 PM
MaskRay accepted this revision.Jul 9 2020, 2:56 PM

LG with some nits

clang/lib/Frontend/CompilerInvocation.cpp
3285

Delete redundant braces

clang/test/CodeGen/fp-strictfp.cpp
13

Append a punctuation, i.e. _Z12fp_precise_1fff:

18

Delete trailing empty lines

kpn marked 3 inline comments as done.Jul 9 2020, 3:06 PM

Thanks for the reviews and the fast turnaround! I do appreciate it!

This revision was automatically updated to reflect the committed changes.
dang added inline comments.Fri, Jul 10, 6:05 AM
clang/include/clang/Driver/Options.td
1246

A bit late to the party, but can you mark this as MarshallingInfoFlag<"LangOpts->ExpStrictFP", "false> so that if (Args.hasArg(OPT_fexperimental_strict_floating_point)) Opts.ExpStrictFP = true; in CompilerInvocation.cpp gets generated automatically, we also get serializing the option for free this way.

kpn marked an inline comment as done.Fri, Jul 10, 6:11 AM
kpn added inline comments.
clang/include/clang/Driver/Options.td
1246

Assuming this patch makes it to the afternoon, how about I open a new review for this?

dang added inline comments.Fri, Jul 10, 6:24 AM
clang/include/clang/Driver/Options.td
1246

Sure no problem