This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Restructure handling of -ffast-math and similar options
ClosedPublic

Authored by john.brawn on Mar 3 2017, 8:51 AM.

Details

Summary

The way -ffast-math and the various related options to tweak floating-point handling are handled is inflexible and rather confusing. This patch restructures things so that we go through the options adjusting our idea of what's enabled as we go, instead of trying to figure each individual thing out by working backwards from the end, as this makes the behaviour of each individual option more clear.

Doing it this way also means we get gcc-compatible behaviour for when the FAST_MATH and FINITE_MATH_ONLY macros are defined, as they should depend on the final set of features that are enabled and not just on -ffast-math and -ffinite-math-only specifically.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn created this revision.Mar 3 2017, 8:51 AM
john.brawn updated this revision to Diff 91311.Mar 10 2017, 4:30 AM

Rebase on top of recent driver changes.

rengolin added inline comments.Mar 14 2017, 6:17 AM
lib/Driver/ToolChains/Clang.cpp
2320

format

2324

default should be the first item, for readability.

2384

Use LLVM_FALLTHROUGH

2451

This is technically correct, but users will be confused if they choose -ffast-math -ffp-contract=on and not see -ffast-math coming out on the other side.

Also, fp-contract=on doesn't preclude -ffast-math for the languages that support it, so I wouldn't add FpContract to this list at all.

Respond to review comments, and also fix a couple of 80-column violations that I spotted.

rengolin added inline comments.Mar 15 2017, 4:46 AM
lib/Driver/ToolChains/Clang.cpp
2347

Also, when -ffast-math is selected, and -ffp-contract=on, we should actually change it to fast, no?

2356

Missing break?

2451

I've been thinking a bit more about this, and I started wondering, why do we even need to pass -ffast-math down?

If all the others are already being passed, shouldn't this flag be redundant?

Finally, we could possibly add instead && FpContract != "off". Would that be better?

john.brawn added inline comments.Mar 15 2017, 5:45 AM
lib/Driver/ToolChains/Clang.cpp
2347

Do you mean "clang -ffp-contract=on -ffast-math should set fp-contract to fast" or "clang -ffast-math -ffp-contract=on should set fp-contract to fast"? The first is already done by the -ffast-math handling below, and I think the second is a bad idea because it violates the principle that later command-line options have priority over earlier ones.

2356

Whoops, will fix.

2451

As the comment above says, -ffast-math enables the FAST_MATH macro.

As to FpContract, going back and checking gcc setting -ffp-contract has no effect on whether FAST_MATH is defined so I think just not checking FpContract here is correct.

rengolin accepted this revision.Mar 15 2017, 6:14 AM

Ok, with the break fix, this looks goof to me. Thanks!

lib/Driver/ToolChains/Clang.cpp
2347

The former is done by overall design, the latter is different to most other options because of what "on" means and how it's *not* implemented, meaning it'll be effectively "off". Otherwise, I'd completely agree with you.

But that's not a strong point, for me. I'm happy to not have that now and involve this into a more general "what does on mean" later on.

2451

Right, makes sense.

This revision is now accepted and ready to land.Mar 15 2017, 6:14 AM
This revision was automatically updated to reflect the committed changes.