[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

andrew.w.kaylor added a comment. In D72675#1920309 , @lebedev.ri wrote: > I may be wrong, but i suspect those failures aren't actually due to the fact > that we pessimize optimizations with this change, but that the whole > execution > just fails. Can

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

lebedev.ri added a comment. I may be wrong, but i suspect those failures aren't actually due to the fact that we pessimize optimizations with this change, but that the whole execution just fails. Can you try running test-suite locally? Do tests themselves actually pass, ignoring the question of

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

mibintc added a comment. @David.Bolvansky told me “https://lnt.llvm.org/ -> Test suite nts -> watch for > LNT-Broadwell-AVX2-O3__clang_PROD__x86_64:1364” The fails were seen on aarch too and a couple other arch. AFAIK the old results are no longer availble. i scraped the list of fails,

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

spatel added a comment. In D72675#1919793 , @mibintc wrote: > In D72675#1919685 , @spatel wrote: > > > In D72675#1917844 , @wristow wrote: > > > > > Revisiting this patch.

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

mibintc added a comment. This patch, which hasn't been committed, contains modifications to the UserManual with many details concerning 'floating point semantic modes" and the relation to floating point command line options. This is from a discussion that @andrew.w.kaylor initiated on the

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

mibintc added a comment. In D72675#1919685 , @spatel wrote: > In D72675#1917844 , @wristow wrote: > > > Revisiting this patch. I think that before fixing the `-ffp-contract=off` > > problem I originally raised

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

spatel added a subscriber: mibintc. spatel added a comment. In D72675#1917844 , @wristow wrote: > Revisiting this patch. I think that before fixing the `-ffp-contract=off` > problem I originally raised here, there are two questions that have come up >

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow added a comment. Revisiting this patch. I think that before fixing the `-ffp-contract=off` problem I originally raised here, there are two questions that have come up in this discussion that we should first resolve. Specifically: (1) Do we want to enable FMA transformations by

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

uweigand added a comment. In D72675#1829866 , @hfinkel wrote: > > I'm not sure whether this is deliberate (but it seems weird) or just a bug. > > I can ask the GCC developers ... > > Please do. If there's a rationale, we should know. Sorry for the

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow marked 4 inline comments as done. wristow added inline comments. Comment at: clang/test/Driver/fast-math.c:196 +// RUN: %clang -### -ffast-math -ffp-contract=fast -c %s 2>&1 \ +// RUN: | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s +// wristow wrote:

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow updated this revision to Diff 240689. wristow added a comment. Used the clearer '!off && !on' (rather than '!(off || on)') in the checks. Added more tests that note the current situation that `-ffast-math` enables FMA. overriding an earlier switch that had disabled it (included a "TODO"

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow added a comment. About: >> This is a bit of an oddity in our handling. > > Yes it is! > > This is certainly getting more complicated than I had originally expected. I > feel there isn't a clear spec on what we want in terms of whether FMA should > be enabled "automatically" at (for

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow marked 4 inline comments as done. wristow added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2792 if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && - !TrappingMath) + !TrappingMath && !(FPContract.equals("off") ||

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

andrew.w.kaylor added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2792 if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && - !TrappingMath) + !TrappingMath && !(FPContract.equals("off") || FPContract.equals("on")))

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow updated this revision to Diff 240305. wristow added a comment. Update a comment to remove the discussion about enabling the `__FAST_MATH__` preprocessor macro. The handling of the setting of `__FAST_MATH__` is done in "clang/lib/Frontend/InitPreprocessor.cpp", and once we decide on the

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow added a comment. In D72675#1839662 , @andrew.w.kaylor wrote: > In D72675#1839492 , @wristow wrote: > > > 1. Should we enable FMA "by default" at (for example) '-O2'? > > > We recently introduced a new

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

andrew.w.kaylor added a comment. In D72675#1839492 , @wristow wrote: > 1. Should we enable FMA "by default" at (for example) '-O2'? We recently introduced a new option "-ffp-model=[precise|fast|strict], which is supposed to serve as an umbrella option

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow added a comment. > A separate question is the interaction of `-ffast-math` with > `-ffp-contract=`. Currently, there is no such interaction whatsoever in GCC: > `-ffast-math` does not imply any particular `-ffp-contract=` setting, and > vice versa the `-ffp-contract=` setting is not

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

hfinkel added a comment. > I'm not sure whether this is deliberate (but it seems weird) or just a bug. I > can ask the GCC developers ... Please do. If there's a rationale, we should know. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72675/new/ https://reviews.llvm.org/D72675

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

uweigand added a comment. I've had a quick look at GCC, and it seems there's a couple of different issues. First of all, `-ffast-math` is a "collective" flag that has no separate meaning except setting a bunch of other flags. Currently, these flags are: `-fno-math-errno`, `

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

spatel added a comment. In D72675#1827893 , @wristow wrote: > How to handle this seems like an implementation question. The code here is > just deciding whether or not we intend to pass "-ffast-math" to cc1 (it isn't > directly defining

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow updated this revision to Diff 238933. wristow added a comment. Updated patch to correct a comment and fix a typo. Regarding the point from @spatel : > This follows the reasoning from the earlier discussion, but after re-reading > the gcc comment in particular, I'm wondering whether

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

spatel added reviewers: efriedma, scanon, arsenm, echristo, RKSimon. spatel added a comment. Herald added a subscriber: wdng. Adding potential FP reviewers for question of gcc- (potentially-buggy-) compatibility. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72675/new/

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

spatel added a comment. This follows the reasoning that we discussed in the earlier discussion, but after re-reading the gcc comment in particular, I'm wondering whether this is what we really want to do... If __FAST_MATH__ is purely here for compatibility with gcc, then should we mimic gcc

[PATCH] D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction

wristow updated this revision to Diff 238595. wristow retitled this revision from "Fix -ffast-math/-ffp-contract interaction" to "[Clang][Driver] Fix -ffast-math/-ffp-contract interaction". wristow added a comment. Changing this to address only the Clang driver aspect, as discussed in the