This is an archive of the discontinued LLVM Phabricator instance.

Remove connection between 'ffast-math' and 'ffp-contract'.
ClosedPublic

Authored by zahiraam on Apr 12 2022, 1:39 PM.

Details

Summary

Currently the options ‘ffast-math’ and ‘ffp-contract’ are connected. When ‘ffast-math’ is set, ffp-contract is altered this way:
-ffast-math/ Ofast -> ffp-contract=fast
-fno-fast-math -> if ffp-contract= fast then ffp-contract=on else ffp-contract unchanged

This differs from gcc which doesn’t connect the two options.

Connecting these two options in clang, resulted in spurious warnings when the user combines these two options -ffast-math -fno-fast-math; see issue https://github.com/llvm/llvm-project/issues/54625.

The issue is that the ‘ffast-math’ option is an on/off flag, but the ‘ffp-contract’ is an on/off/fast flag. So when ‘fno-fast-math’ is used there is no obvious value for ‘ffp-contract’. What should the value of ffp-contract be for -ffp-contract=fast -fno-fast-math and -ffast-math -ffp-contract=fast -fno-fast-math? The current logic sets ffp-contract back to on in these cases. This doesn’t take into account that the value of ffp-contract is modified by an explicit ffp-contract` option.
This patch is proposing a set of rules to apply when ffp-contract', ffast-math and fno-fast-math are combined. These rules would give the user the expected behavior and no diagnostic would be needed.

See RFC https://discourse.llvm.org/t/rfc-making-ffast-math-option-unrelated-to-ffp-contract-option/61912

Diff Detail

Event Timeline

zahiraam created this revision.Apr 12 2022, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:39 PM
zahiraam requested review of this revision.Apr 12 2022, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:39 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

I think these changes should go through a community RFC on Discourse given that they alter the behavior of existing flags.

zahiraam retitled this revision from Remove connection between 'ffast-math' and 'ffp-contract'. to [WIP] Remove connection between 'ffast-math' and 'ffp-contract'..Apr 13 2022, 6:38 AM
zahiraam edited the summary of this revision. (Show Details)Apr 20 2022, 9:07 AM
zahiraam edited the summary of this revision. (Show Details)Aug 29 2022, 7:14 AM
zahiraam added a reviewer: fhahn.
zahiraam edited the summary of this revision. (Show Details)
zahiraam edited the summary of this revision. (Show Details)Aug 29 2022, 7:16 AM
zahiraam edited the summary of this revision. (Show Details)
zahiraam updated this revision to Diff 456337.Aug 29 2022, 7:25 AM
zahiraam retitled this revision from [WIP] Remove connection between 'ffast-math' and 'ffp-contract'. to Remove connection between 'ffast-math' and 'ffp-contract'..Sep 1 2022, 12:35 PM
zahiraam updated this revision to Diff 457358.Sep 1 2022, 12:44 PM

@fhahn @aaron.ballman would you mind taking time for a review for this patch? Thanks.

I made some suggested documentation changes, but would love to hear more from folks heavily into floating-point options.

clang/docs/UsersManual.rst
1409–1410
1432–1435
1441–1443
1450–1452
zahiraam updated this revision to Diff 458506.Sep 7 2022, 10:35 AM
zahiraam marked 4 inline comments as done.

Some general comments on the documentation here:

One of the lesser-known issues of -ffast-math is the fact that it (or -funsafe-math-optimizations) causes crtfastmath.o to be linked, which adds a static constructor that sets the FTZ/DAZ bits in MXCSR, affecting not only the current compilation unit but all static and shared libraries included in the program. This behavior deserves to be mentioned somewhere in the documentation. Additionally, the effects of fast-math/unsafe-math-optimizations on fdenormal-fp-math{32} should be noted. I realize this is orthogonal to the actual change you're doing here, but while we're improving the documentation of -f[no-]fast-math, we should probably fix this missing documentation at the same time.

clang/lib/Driver/ToolChains/Clang.cpp
2778

Given that this will only be set to "" or "fast", it probably makes sense to make for this be a bool SeenFfastMathOption variable instead.

zahiraam updated this revision to Diff 459028.Sep 9 2022, 6:10 AM
zahiraam marked an inline comment as done.
jcranmer-intel added inline comments.Sep 13 2022, 2:24 PM
clang/docs/UsersManual.rst
1430

You can add -fdenormal-fp-math=ieee here.

1453–1455

You can replace this text with saying that -fno-fast-math implies -fdenormal-fp-math=ieee. No need to directly mention DenormalFPMath; instead relate it to the other command line flags that are documented.

1455–1458

crtfastmath.o should probably be mentioned in a separate section, like the "A note about ..." sections, with the text in -fno-fast-math only mentioning that it causes code not to be linked with crtfastmath.o.

zahiraam updated this revision to Diff 459893.Sep 13 2022, 3:12 PM
zahiraam marked 2 inline comments as done.
zahiraam added inline comments.
clang/docs/UsersManual.rst
1453–1455

Not sure that's the change you are proposing?

zahiraam updated this revision to Diff 460178.Sep 14 2022, 12:11 PM
jcranmer-intel accepted this revision.Sep 14 2022, 1:57 PM

I'm happy with these changes. I'll let Aaron have one last crack at the wording of the documentation, in case there's any minor editorial stuff he'd like to see cleaned up.

This revision is now accepted and ready to land.Sep 14 2022, 1:57 PM
zahiraam updated this revision to Diff 460389.Sep 15 2022, 6:17 AM
aaron.ballman accepted this revision.Sep 15 2022, 8:17 AM

LGTM aside from a few nits.

clang/docs/UsersManual.rst
1406
1455
1689
zahiraam updated this revision to Diff 460470.Sep 15 2022, 11:52 AM
zahiraam marked 3 inline comments as done.
lenary added a subscriber: lenary.Nov 1 2022, 9:59 AM

Reverse ping. This has been accepted, what is the status of landing this?

Reverse ping. This has been accepted, what is the status of landing this?

As far as I can tell this has landed.

zahiraam set the repository for this revision to rG LLVM Github Monorepo.Nov 1 2022, 10:25 AM

Oh, can you close this revision and link it to the landed commit please?

zahiraam closed this revision.Nov 1 2022, 10:42 AM