This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add -ffast-math and -Ofast
ClosedPublic

Authored by tblah on Nov 24 2022, 8:22 AM.

Details

Summary

RFC: https://discourse.llvm.org/t/rfc-the-meaning-of-ofast/66554

-Ofast is defined as -O3 -ffast-math

-ffast-math will mean the following:

  • -fno-honor-infinities
  • -fno-honor-nans
  • -fassociative-math
  • -freciprocal-math
  • -fapprox-func
  • -fno-signed-zeros
  • -ffp-contract=fast

This definition matches the meaning of the "fast" attribute in LLVM IR: https://llvm.org/docs/LangRef.html#fast-math-flags

-fstack-arrays should also be enabled with -Ofast but that flang is not yet implemented and so has been left for a future patch.

clang -cc1 accepts -Ofast. I did not add -Ofast to flang-new -fc1 because it is redundant because the compiler driver will always resolve -Ofast into -O3 -ffast-math (I added a test for this in flang/test/Driver/fast_math.f90; there is a test that -ffast-math leads to the fast attribute in flang/test/Lower/fast-math-arithmetic.f90). This means that flang-new -Ofast will be forwarded as flang-new -fc1 -O3 -ffast-math, which is entirely equivalent. Avoiding adding the umbrella flag to flang-new -fc1 avoids unnecessary complexity in the frontend driver.

-menable-infs is removed from the frontend-forwarding test because if all of the fast-math component flags are present, these will be resolved into the fast-math flag. Instead -menable-infs is tested in the fast-math test.

-fno-fast-math will not make FPContract less strict: -ffp-contract=off -fno-fast-math will not reset to -ffp-contract=fast (the default).

Diff Detail

Event Timeline

tblah created this revision.Nov 24 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
tblah requested review of this revision.Nov 24 2022, 8:22 AM
tblah edited the summary of this revision. (Show Details)
tblah edited the summary of this revision. (Show Details)Nov 24 2022, 8:38 AM

Hi Tom, thanks for working on this! I'm not that familiar with the semantics of -Ofast (and the related flags), but everything else looks good. Some comments below and inline.

-Ofast is defined as -O3 -ffast-math

In the compiler driver,flang-new, or the frontend driver, flang-new -fc1?

This definition matches the meaning of the "fast" attribute in LLVM IR: https://llvm.org/docs/LangRef.html#fast-math-flags

[nit] But there's not guarantee just yet that -ofast will result in the attributes being added? In fact, why not add a test to verify that?

Another related question - are the semantics that you are implementing here identical to Clang? Either way, could you document?

clang -cc1 accepts -Ofast. I did not add it to flang -fc1 because this seems redundant because the compiler driver will always resolve -Ofast into -O3 -ffast-math (I added a test for this).

I don't quite follow - Clang users can run clang -cc1 -Ofast. What's the equivalent in Flang?

-fstack-arrays should also be enabled with -Ofast but that flang is not yet implemented and so has been left for a future patch.

Why not add a test?

flang/lib/Frontend/CompilerInvocation.cpp
733

Why is this diagnostic needed? And is it tested?

flang/test/Driver/fast_math.f90
8–9

Similar suggestion for other tests in this file (whenever it makes sense)

tblah added a comment.Nov 28 2022, 1:16 AM

Hi Tom, thanks for working on this! I'm not that familiar with the semantics of -Ofast (and the related flags), but everything else looks good. Some comments below and inline.

-Ofast is defined as -O3 -ffast-math

In the compiler driver,flang-new, or the frontend driver, flang-new -fc1?

In the compiler driver. flang-new -fc1 does not accept -Ofast.

This definition matches the meaning of the "fast" attribute in LLVM IR: https://llvm.org/docs/LangRef.html#fast-math-flags

[nit] But there's not guarantee just yet that -ofast will result in the attributes being added? In fact, why not add a test to verify that?

There is a test checking that -Ofast does generate -ffast-math (flang/test/Driver/fast_math.f90), and that -ffast-math leads to FIR with the fast attribute (flang/test/Lower/fast-math-arithmetic.f90).

Another related question - are the semantics that you are implementing here identical to Clang? Either way, could you document?

clang -cc1 accepts -Ofast. I did not add it to flang -fc1 because this seems redundant because the compiler driver will always resolve -Ofast into -O3 -ffast-math (I added a test for this).

I don't quite follow - Clang users can run clang -cc1 -Ofast. What's the equivalent in Flang?

I guess the equivalent in Flang would be flang-new -cc1 -O3 -ffast-math. As I understand it, flang-new -fc1 is not intended as an interface for users.

-fstack-arrays should also be enabled with -Ofast but that flang is not yet implemented and so has been left for a future patch.

Why not add a test?

Because that flag has not been added to Flang yet

tblah added inline comments.Nov 28 2022, 1:21 AM
flang/lib/Frontend/CompilerInvocation.cpp
733

@vzakhari didn't want to remove these warnings when support was first added because the intrinsics simplification pass still ignored fastmath (https://reviews.llvm.org/D137391#3908424).

There has been work on the intrinsics since then in https://reviews.llvm.org/D138048 but I don't know if this is complete. @vzakhari would you like me to remove these warnings for the implemented flags now?

The test is at the end of flang/test/Driver/fast_math.f90

tblah updated this revision to Diff 478159.Nov 28 2022, 1:54 AM

Update tests to check that forwarded flags are on the same line as -fc1

tblah marked an inline comment as done.Nov 28 2022, 1:54 AM

Hi Tom, thanks for working on this! I'm not that familiar with the semantics of -Ofast (and the related flags), but everything else looks good. Some comments below and inline.

-Ofast is defined as -O3 -ffast-math

In the compiler driver,flang-new, or the frontend driver, flang-new -fc1?

In the compiler driver. flang-new -fc1 does not accept -Ofast.

So, -Ofast in flang-new is translated as flang-new -fc1 -O3 -ffast-math? Could you clarify this in the summary? Otherwise, there is no documentation for this.

This definition matches the meaning of the "fast" attribute in LLVM IR: https://llvm.org/docs/LangRef.html#fast-math-flags

[nit] But there's not guarantee just yet that -ofast will result in the attributes being added? In fact, why not add a test to verify that?

There is a test checking that -Ofast does generate -ffast-math (flang/test/Driver/fast_math.f90), and that -ffast-math leads to FIR with the fast attribute (flang/test/Lower/fast-math-arithmetic.f90).

Worth adding a note in the summary.

Another related question - are the semantics that you are implementing here identical to Clang? Either way, could you document?

clang -cc1 accepts -Ofast. I did not add it to flang -fc1 because this seems redundant because the compiler driver will always resolve -Ofast into -O3 -ffast-math (I added a test for this).

So what does clang -cc1 -Ofast mean? And how come we don't need this in flang-new -fc1?

I don't quite follow - Clang users can run clang -cc1 -Ofast. What's the equivalent in Flang?

I guess the equivalent in Flang would be flang-new -cc1 -O3 -ffast-math. As I understand it, flang-new -fc1 is not intended as an interface for users.

I would recommend following the principle of least surprise and just follow what Clang does.

-fstack-arrays should also be enabled with -Ofast but that flang is not yet implemented and so has been left for a future patch.

Why not add a test?

Because that flag has not been added to Flang yet

I don't follow - you can still write a test for a flag that's not supported.

vzakhari added inline comments.Nov 28 2022, 4:26 PM
flang/include/flang/Common/MathOptionsBase.def
42 ↗(On Diff #478159)

I do not think this is needed here. In LLVM fast is just a decoration for "all-bits-set", so I do not see why we would need a separate bit for this. Moreover, if you add it we will have to guarantee and verify consistency (i.e. that FastMath == all-above-bits-are-set).

flang/lib/Frontend/CompilerInvocation.cpp
733

Yes, I think we can remove the warnings now.

tblah updated this revision to Diff 478508.Nov 29 2022, 3:47 AM
tblah edited the summary of this revision. (Show Details)

Update to remove the warning that -ffast-math is unimplemented and do not store fast-math status independently of the flags it contains.

tblah added inline comments.Nov 29 2022, 3:48 AM
flang/lib/Frontend/CompilerInvocation.cpp
733

I have removed the rest of the warnings in https://reviews.llvm.org/D138907

tblah edited the summary of this revision. (Show Details)Nov 29 2022, 5:11 AM
tblah updated this revision to Diff 478588.Nov 29 2022, 7:19 AM

Add test for -fstack-arrays usage warning

awarzynski accepted this revision.Nov 29 2022, 8:25 AM

Thanks for the updates. A few more nits from me. LGTM and ready to land once addressed.

@vzakhari ?

clang/lib/Driver/ToolChains/Flang.cpp
197–200

So this means that:

  • --ffp-contract=off -fno-fast-math --> FPContract = "off"
  • --fno-fast-math --> FPContract = ""?

I would reduce this comment to that ^^^. In particular, what if this is no longer true: "// FPContract = fast is the default anyway" ?

Also, could you mention this in the summary? This a design decision worth documenting.

213

Is this comment needed? Would other flags do you have in mind?

This revision is now accepted and ready to land.Nov 29 2022, 8:25 AM
tblah updated this revision to Diff 478873.Nov 30 2022, 3:04 AM

Updated comment, updated patch context, fixed unused variable

tblah edited the summary of this revision. (Show Details)Nov 30 2022, 8:28 AM
tblah updated this revision to Diff 481203.Dec 8 2022, 2:00 AM

Updating patch add documentation of the meaning of -Ofast, -ffast-math, and comparison with gfortran and nvfortran.

Also added a test to ensure that crtfastmath.o is linked when -ffast-math is specified to the compiler driver.

tblah updated this revision to Diff 481608.Dec 9 2022, 5:34 AM

Update patch to skip the test for linking to crtfastmath.o on Windows as there doesn't seem to be a Windows equivalent of crtfastmath.o.

tblah marked 4 inline comments as done.Dec 9 2022, 5:35 AM
tblah added a comment.EditedDec 9 2022, 6:24 AM

Failed Tests (5):

Flang :: Fir/boxproc.fir
Flang :: Fir/target-rewrite-arg-position.fir
Flang :: Fir/target-rewrite-boxchar.fir
Flang :: Fir/target-rewrite-char-proc.fir
Flang :: Fir/target.fir

I am getting the same without my patch (upstream ref 38e87e8af47a)
 

kiranchandramohan accepted this revision.Dec 9 2022, 8:08 AM

LGTM. Thanks for addressing all the comments and comparing the option with other Fortran compilers. Please wait a day before submitting in case there are other comments.

flang/docs/FlangDriver.md
552

Nit: Add a link to what these options mean. Possibly say that these options are required to set the various fast math attributes on the LLVM IR instructions as listed in the following link.
https://llvm.org/docs/LangRef.html#fast-math-flags

588

Nit: You can say that the O3 pipeline has passes that perform transformations like inlining, vectorisation, unrolling etc. Additionally the GVN, LICM passes performs redundancy elimination like Mpre, Mlre.

590

Nit: I guess the plan is not to include this flag even when fomit-frame-pointer support is added.

Thank you for the additional changes, @tblah! LGTM

tblah updated this revision to Diff 481681.Dec 9 2022, 10:18 AM

Update with nitpicks to documentation (mostly to re-run Windows CI)

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 9 2022, 11:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
clementval added a subscriber: clementval.EditedDec 12 2022, 1:27 AM

I have the Driver/fast_math.f90 failing on my side.

llvm-project/flang/test/Driver/fast_math.f90:63:14: error: CHECK-CRT: expected string not found in input
! CHECK-CRT: crtbeginS.o

I have a crtbegin.o in the link line but not a crtbeginS.o

tblah added a comment.Dec 12 2022, 3:30 AM

I have the Driver/fast_math.f90 failing on my side.

llvm-project/flang/test/Driver/fast_math.f90:63:14: error: CHECK-CRT: expected string not found in input
! CHECK-CRT: crtbeginS.o

I have a crtbegin.o in the link line but not a crtbeginS.o

Thanks for the heads up. This should be fixed by https://github.com/llvm/llvm-project/commit/bea45027b43ec61a3efc4b487ceca2da25504432

I have the Driver/fast_math.f90 failing on my side.

llvm-project/flang/test/Driver/fast_math.f90:63:14: error: CHECK-CRT: expected string not found in input
! CHECK-CRT: crtbeginS.o

I have a crtbegin.o in the link line but not a crtbeginS.o

Thanks for the heads up. This should be fixed by https://github.com/llvm/llvm-project/commit/bea45027b43ec61a3efc4b487ceca2da25504432

Thanks for the fix. crtendS.o has the same issue.

amyk added a subscriber: amyk.Dec 12 2022, 11:11 AM

Hi,

The ppc64le-flang-rhel-clang bot is also experiencing some failures with the Driver/fast_math.f90 test case. In particular, crtfastmath.o is not found at all for CHECK-CRT as we can see in the failure details.

llvm-project/flang/test/Driver/fast_math.f90:64:19: error: CHECK-CRT-SAME: expected string not found in input
! CHECK-CRT-SAME: crtfastmath.o

The output for the CHECK-CRT line on the bot looks like the following:

"/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/bin/flang-new" "-fc1" "-triple" "powerpc64le-unknown-linux-gnu" "-emit-obj" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-ffast-math" "-target-cpu" "ppc64le" "-o" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-x" "f95-cpp-input" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/llvm-project/flang/test/Driver/fast_math.f90" 

"/usr/bin/ld" "-pie" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf64lppc" "-dynamic-linker" "/lib64/ld64.so.2" "-o" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/tools/flang/test/Driver/Output/fast_math.f90.tmp" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/Scrt1.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crti.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtbeginS.o" "-L/usr/lib/gcc/ppc64le-redhat-linux/8" "-L/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64" "-L/lib/../lib64" "-L/usr/lib/../lib64" "-L/lib" "-L/usr/lib" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-lFortran_main" "-lFortranRuntime" "-lFortranDecimal" "-lm" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtendS.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crtn.o"

Would it be possible to follow up with a fix for this test case?

Hi,

The ppc64le-flang-rhel-clang bot is also experiencing some failures with the Driver/fast_math.f90 test case. In particular, crtfastmath.o is not found at all for CHECK-CRT as we can see in the failure details.

llvm-project/flang/test/Driver/fast_math.f90:64:19: error: CHECK-CRT-SAME: expected string not found in input
! CHECK-CRT-SAME: crtfastmath.o

The output for the CHECK-CRT line on the bot looks like the following:

"/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/bin/flang-new" "-fc1" "-triple" "powerpc64le-unknown-linux-gnu" "-emit-obj" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-ffast-math" "-target-cpu" "ppc64le" "-o" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-x" "f95-cpp-input" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/llvm-project/flang/test/Driver/fast_math.f90" 

"/usr/bin/ld" "-pie" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf64lppc" "-dynamic-linker" "/lib64/ld64.so.2" "-o" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/tools/flang/test/Driver/Output/fast_math.f90.tmp" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/Scrt1.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crti.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtbeginS.o" "-L/usr/lib/gcc/ppc64le-redhat-linux/8" "-L/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64" "-L/lib/../lib64" "-L/usr/lib/../lib64" "-L/lib" "-L/usr/lib" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-lFortran_main" "-lFortranRuntime" "-lFortranDecimal" "-lm" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtendS.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crtn.o"

Would it be possible to follow up with a fix for this test case?

Thanks for letting me know. I thought I had already disabled that test on powerpc (in https://github.com/llvm/llvm-project/commit/20cd3153f3775fcdc1eeeb54062849eead51e24a) but apparently it didn't work. I don't have a powerpc system available to test this. I have pushed a second attempt at https://github.com/llvm/llvm-project/commit/6442b4da4e7018e8f264965768b9e4fdee393c8f. Please let me know if that works.

amyk added a comment.Dec 12 2022, 12:58 PM

Hi,

The ppc64le-flang-rhel-clang bot is also experiencing some failures with the Driver/fast_math.f90 test case. In particular, crtfastmath.o is not found at all for CHECK-CRT as we can see in the failure details.

llvm-project/flang/test/Driver/fast_math.f90:64:19: error: CHECK-CRT-SAME: expected string not found in input
! CHECK-CRT-SAME: crtfastmath.o

The output for the CHECK-CRT line on the bot looks like the following:

"/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/bin/flang-new" "-fc1" "-triple" "powerpc64le-unknown-linux-gnu" "-emit-obj" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-ffast-math" "-target-cpu" "ppc64le" "-o" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-x" "f95-cpp-input" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/llvm-project/flang/test/Driver/fast_math.f90" 

"/usr/bin/ld" "-pie" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf64lppc" "-dynamic-linker" "/lib64/ld64.so.2" "-o" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/tools/flang/test/Driver/Output/fast_math.f90.tmp" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/Scrt1.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crti.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtbeginS.o" "-L/usr/lib/gcc/ppc64le-redhat-linux/8" "-L/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64" "-L/lib/../lib64" "-L/usr/lib/../lib64" "-L/lib" "-L/usr/lib" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-lFortran_main" "-lFortranRuntime" "-lFortranDecimal" "-lm" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtendS.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crtn.o"

Would it be possible to follow up with a fix for this test case?

Thanks for letting me know. I thought I had already disabled that test on powerpc (in https://github.com/llvm/llvm-project/commit/20cd3153f3775fcdc1eeeb54062849eead51e24a) but apparently it didn't work. I don't have a powerpc system available to test this. I have pushed a second attempt at https://github.com/llvm/llvm-project/commit/6442b4da4e7018e8f264965768b9e4fdee393c8f. Please let me know if that works.

Thanks for the follow up patch! I tested the patch locally and also saw the buildbot results, and it doesn't appear like the follow up patch marked powerpc as unsupported as the error still persists (https://lab.llvm.org/buildbot/#/builders/21/builds/57850).
I was playing around with the test case locally and what appears to work is doing something like:

! UNSUPPORTED: powerpc-registered-target
tblah added a comment.Dec 12 2022, 1:51 PM

Thanks for the follow up patch! I tested the patch locally and also saw the buildbot results, and it doesn't appear like the follow up patch marked powerpc as unsupported as the error still persists (https://lab.llvm.org/buildbot/#/builders/21/builds/57850).
I was playing around with the test case locally and what appears to work is doing something like:

! UNSUPPORTED: powerpc-registered-target

Thanks https://github.com/llvm/llvm-project/commit/9d86f2dc4f1d2e4e1a991be82384bbdb310f0618

amyk added a comment.Dec 12 2022, 2:18 PM

Thanks for the follow up patch! I tested the patch locally and also saw the buildbot results, and it doesn't appear like the follow up patch marked powerpc as unsupported as the error still persists (https://lab.llvm.org/buildbot/#/builders/21/builds/57850).
I was playing around with the test case locally and what appears to work is doing something like:

! UNSUPPORTED: powerpc-registered-target

Thanks https://github.com/llvm/llvm-project/commit/9d86f2dc4f1d2e4e1a991be82384bbdb310f0618

Thanks for following up! The bot is now green (https://lab.llvm.org/buildbot/#/builders/21/builds/57857).

See D139967 for why UNSUPPORTED: powerpc didn't work. That patch will put it back, and also update the lit config so the check will work now.

tblah added a comment.Dec 14 2022, 8:34 AM

See D139967 for why UNSUPPORTED: powerpc didn't work. That patch will put it back, and also update the lit config so the check will work now.

Thanks for the update!

amyk added a comment.Dec 14 2022, 8:42 AM

See D139967 for why UNSUPPORTED: powerpc didn't work. That patch will put it back, and also update the lit config so the check will work now.

Thank you for following up with this. I appreciate it!

mnadeem added inline comments.
flang/test/Driver/fast_math.f90
60–63

I think this test should use an explicit sysroot like the clang test does. It fails on my system (x86 linux, llvm built for arm target) because it cant find the crtfastmath.o library.

// file linux-ld.c

RUN: --sysroot=%S/Inputs/basic_linux_tree 2>&1 \
RUN: | FileCheck --check-prefix=CHECK-CRTFASTMATH %s

tblah marked an inline comment as done.Jan 28 2023, 5:54 AM
tblah added inline comments.
flang/test/Driver/fast_math.f90
60–63

Thanks for the suggestion https://reviews.llvm.org/D142807