Page MenuHomePhabricator

Remove redundant option '-menable-unsafe-fp-math'.
ClosedPublic

Authored by zahiraam on Oct 3 2022, 12:36 PM.

Details

Summary

There are currently two options that are used to tell the compiler to perform unsafe floating-point optimizations: '-ffast-math' and '-funsafe-math-optimizations'.

'ffast-math' is enabled by default. It automatically enables the driver option '-menable-unsafe-fp-math'.
Below is a table illustrating the special operations enabled automatically by '-ffast-math', '-funsafe-math-optimizations' and '-menable-unsafe-fp-math' respectively.

Special Operations-ffast-math-funsafe-math-optimizations-menable-unsafe-fp-math
MathErrno011
FiniteMathOnly100
AllowFPReassoc111
NoSignedZero111
AllowRecip111
ApproxFunc111
RoundingMath000
UnsafeFPMath101
FPContractfastonon

'-ffast-math' enables '-fno-math-errno', '-ffinite-math-only', '-funsafe-math-optimzations' and sets 'FpContract' to 'fast'. The driver option
'-menable-unsafe-fp-math' enables the same special options than '-funsafe-math-optimizations'. This is redundant. We propose to remove the driver
option '-menable-unsafe-fp-math' and use instead, the setting of the special operations to set the function attribute 'unsafe-fp-math'. This attribute will
be enabled only if those special operations are enabled and if 'FPContract' is either 'fast' or set to the default value.

Diff Detail

Event Timeline

zahiraam created this revision.Oct 3 2022, 12:36 PM
zahiraam created this object with visibility "No One".
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 12:36 PM
zahiraam requested review of this revision.Oct 3 2022, 12:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 12:36 PM
zahiraam edited the summary of this revision. (Show Details)Oct 3 2022, 12:37 PM
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)Oct 3 2022, 12:40 PM
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)Oct 3 2022, 12:43 PM
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)
zahiraam changed the visibility from "No One" to "Custom Policy".Oct 3 2022, 12:46 PM
zahiraam updated this revision to Diff 465768.Oct 6 2022, 9:23 AM
zahiraam changed the visibility from "Custom Policy" to "Public (No Login Required)".
dexonsmith resigned from this revision.Oct 6 2022, 10:35 AM
MaskRay added a comment.EditedOct 6 2022, 10:54 AM

Thanks for noticing the issue. There are some formatting issues in the description:) And a typo in the table cell initeMathOnly

-menable-unsafe-fp-math looks like a CC1 for a long time, probably from 2012. In 2020 D82574 accidentally made it a driver option. (clang<=11 does not have the option. I haven't checked 12.)
This option does not exist in GCC and I can't find non-clang-internal uses of the option. I agree we should remove it.

zahiraam edited the summary of this revision. (Show Details)Oct 6 2022, 11:03 AM
zahiraam edited the summary of this revision. (Show Details)
zahiraam updated this revision to Diff 465790.Oct 6 2022, 11:05 AM

I'm also okay with this direction. I took a look to see if people seemed to be using this option in their build scripts (maybe we would need a louder deprecation period), and it seems like most of the uses out there are in forks of Clang. Once I excluded things that looked too clang-like, I spotted: https://sourcegraph.com/search?q=context:global+-file:.*test.*+-file:.*clang.*+-file:Tools.cpp+-menable-unsafe-fp-math&patternType=standard -- I don't have the impression we need a deprecation period for this. (Do we consider this to be a potentially breaking change we need to list in the release notes/announcements/clang-vendors?) The changes should have a release note, regardless of what heading we put it under.

I'm also okay with this direction. I took a look to see if people seemed to be using this option in their build scripts (maybe we would need a louder deprecation period), and it seems like most of the uses out there are in forks of Clang. Once I excluded things that looked too clang-like, I spotted: https://sourcegraph.com/search?q=context:global+-file:.*test.*+-file:.*clang.*+-file:Tools.cpp+-menable-unsafe-fp-math&patternType=standard -- I don't have the impression we need a deprecation period for this. (Do we consider this to be a potentially breaking change we need to list in the release notes/announcements/clang-vendors?) The changes should have a release note, regardless of what heading we put it under.

@aaron.ballman There is a chapter in the RN called "Removed Compiler Flags". Some text should definitely be added there. I let other people chime in for the "potentially breaking" in the RN or somewhere else.

zahiraam updated this revision to Diff 466474.Oct 10 2022, 5:41 AM
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)Oct 10 2022, 5:46 AM
aaron.ballman added a reviewer: Restricted Project.Oct 13 2022, 7:03 AM

Adding clang-vendors because of the potential for disruption from the changes.

clang/docs/ReleaseNotes.rst
601–603

Because we diagnose unknown driver flags as an error (https://godbolt.org/z/4xjzKh4Ej) and there's no deprecation period, I think we should put this under the potentially breaking changes section. In this case, I'm specifically worried about proprietary projects using the flag for optimization purposes (a lot of numerical analysis code is behind closed doors).

CC @MaskRay just to make sure there's agreement (we're still trying to figure out what constitutes a breaking change we want to be loud about in terms of driver flags).

Assuming Fangrui doesn't disagree, once this lands, please post an announcement about it into https://discourse.llvm.org/c/announce/46 with the clang and potentially-breaking tags (an example of such a post is: https://discourse.llvm.org/t/clang-16-notice-of-potentially-breaking-changes/65562/ though you wouldn't need all that lead-in text).

+wristow who has been tracking this kind of stuff for Sony.

zahiraam added inline comments.Oct 13 2022, 7:25 AM
clang/docs/ReleaseNotes.rst
601–603

@aaron.ballman would it be worth adding a diagnostic for the option we are removing?

aaron.ballman added inline comments.Oct 13 2022, 7:50 AM
clang/docs/ReleaseNotes.rst
601–603

If we're going to deprecate rather than remove, sure. But I think we're okay removing, and I think the default error diagnostic behavior will be sufficient.

MaskRay added inline comments.Oct 13 2022, 3:19 PM
clang/docs/ReleaseNotes.rst
601–603

Since -menable-unsafe-fp-math was only accidentally exposed in 2020 and it's clear not used in the wild, placing this under "Potentially Breaking Changes" seems overkill to me (lengthy entries in release notes also discourage readers).

The driver option `-menable-unsafe-fp-math` has been removed. Passing it, will result in a hard error.

I think Passing it, will result in a hard error. can be removed as the removal clearly indicates that passing it is an error:)

the compiler options `-funsafe-math-optimizations and -ffast-math` are used instead.

Use xxx or xxx instead.

zahiraam updated this revision to Diff 467749.Oct 14 2022, 5:20 AM
zahiraam marked an inline comment as done.Oct 14 2022, 5:22 AM
zahiraam added inline comments.
clang/docs/ReleaseNotes.rst
601–603

@MaskRay Thanks!
I tend to agree about the "Potentially Breaking changes".

aaron.ballman accepted this revision.Oct 14 2022, 6:20 AM

LGTM!

clang/docs/ReleaseNotes.rst
601–603

Great, that works for me. Thank you @MaskRay!

This revision is now accepted and ready to land.Oct 14 2022, 6:20 AM
This revision was landed with ongoing or failed builds.Oct 14 2022, 7:55 AM
This revision was automatically updated to reflect the committed changes.
zahiraam marked an inline comment as done.

The newly added test func-attr.c fails when run in opt mode. If it is expected to run only at a certain optimization level, it should probably include that level in the test itself. I don't think it seriously blocks anything, but would be good to fix.

/usr/local/google/home/saugustine/llvm/build/bin/clang -O2 -c -ffast-math -emit-llvm -S -o - /usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c  | /usr/local/google/home/saugustine/llvm/build/bin/FileCheck /usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c
/usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c:11:11: error: CHECK: expected string not found in input
// CHECK: define{{.*}} float @foo(float noundef %a, float noundef %b) [[FAST_ATTRS:#[0-9]+]]
          ^
<stdin>:1:1: note: scanning from here
; ModuleID = '/usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c'
^
<stdin>:7:1: note: possible intended match here
define dso_local float @foo(float noundef %a, float noundef %b) local_unnamed_addr #0 {
^

Input file: <stdin>
Check file: /usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            1: ; ModuleID = '/usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c' 
check:11'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
            2: source_filename = "/usr/local/google/home/saugustine/llvm/llvm-project/clang/test/CodeGen/func-attr.c" 
check:11'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            3: target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" 
check:11'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            4: target triple = "x86_64-unknown-linux-gnu" 
check:11'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            5:  
check:11'0     ~
            6: ; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone willreturn uwtable 
check:11'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            7: define dso_local float @foo(float noundef %a, float noundef %b) local_unnamed_addr #0 { 
check:11'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:11'1     ?                                                                                        possible intended match
            8: entry: 
check:11'0     ~~~~~~~
            9:  %add = fadd fast float %b, %a 
check:11'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           10:  ret float %add 
check:11'0     ~~~~~~~~~~~~~~~~
           11: } 
check:11'0     ~~
           12:  
check:11'0     ~
            .
            .
            .
>>>>>>
jrtc27 added a subscriber: jrtc27.Oct 14 2022, 4:02 PM

It also will fail if the configured default target triple is one where the default program address space is non-zero, and maybe other things (do any targets do pre-IR name mangling for C?)

kongyi added a subscriber: kongyi.Oct 15 2022, 7:21 AM

It also will fail if the configured default target triple is one where the default program address space is non-zero, and maybe other things (do any targets do pre-IR name mangling for C?)

Our internal builder is broken due to this failing test, and I took the liberty to fix the test in commit f118280b04 by relaxing the variable name check. Feel free to change it if you want a different fix.

michele.scandale added inline comments.
clang/lib/CodeGen/CGCall.cpp
1865–1869

If I look at the clang driver code, -funsafe-math-optimizations is specified on the CC1 command line if

!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && ApproxFunc && !TrappingMath

is true.
This means that it shouldn't matter the value of the floating point contraction, or whether infs/nans are honored or not.

Was there another issue that this specific part of the change addresses?

zahiraam added inline comments.Oct 25 2022, 11:50 AM
clang/lib/CodeGen/CGCall.cpp
1865–1869

This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated. In this case the function shouldn't be marked with the unsafe-fp-math attribute.

clang/lib/CodeGen/CGCall.cpp
1865–1869

This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated. In this case the function shouldn't be marked with the unsafe-fp-math attribute.

If I understand correctly this is because the function attribute unsafe-fp-math from backends perspective allow any kind of contraction?
If so, shouldn't then this be valid only when the fp-contraction mode is fast, given that on means that only in-statement contraction is allowed, and clang handles that by generating llvm.fmuladd to lower expressions like a * b + c in given statement?

What about the !LangOpts.FiniteMathOnly part? I'd would think this shouldn't be checked at all as there are separate function attributes for infs and nans.

1865–1869

This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated. In this case the function shouldn't be marked with the unsafe-fp-math attribute.

zahiraam added inline comments.Oct 26 2022, 6:14 AM
clang/lib/CodeGen/CGCall.cpp
1865–1869

This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated. In this case the function shouldn't be marked with the unsafe-fp-math attribute.

If I understand correctly this is because the function attribute unsafe-fp-math from backends perspective allow any kind of contraction?

That's correct.

If so, shouldn't then this be valid only when the fp-contraction mode is fast, given that on means that only in-statement contraction is allowed, and clang handles that by generating llvm.fmuladd to lower expressions like a * b + c in given statement?

What about the !LangOpts.FiniteMathOnly part? I'd would think this shouldn't be checked at all as there are separate function attributes for infs and nans.

when ffp-contract=on is used with no safe optimizations along with it, the attribute unsafe-fp-math is not set but tryEmitMulAdd will return an FMulAdd. Please keep in mind that this logic hasn't really changed. The only thing that was done was to explicitly expand the operations of unsafe optimizations in the original condition.