clang warning passing "-ffast-math -fno-fast-math"

[efriedma-quic]

$ bin/clang -c x.c -ffast-math -fno-fast-math
clang-13: warning: overriding '-ffp-contract=fast' option with '-ffp-contract=on' [-Woverriding-t-option]
$ bin/clang -c x.c -Ofast -fno-fast-math
clang-13: warning: overriding '-ffp-contract=fast' option with '-ffp-contract=on' [-Woverriding-t-option]

The warning seems unexpected... I don't think it makes sense to warn here? Even if there is some reason to warn here, it's hard to understand the relationship between the warning text and the actual arguments.

I think this is a regression from https://reviews.llvm.org/D107994 .

cc @zahiraam @andykaylor

[llvmbot]

@llvm/issue-subscribers-clang-driver

[zahiraam]

@efriedma-quic I am able to reproduce it . I agree I don't think we need a warning here. @andykaylor do you agree? If you think we should have one, then it should be clearer in the wordings.

[andykaylor]

This is a bit messy. The problem is that -ffast-math is an umbrella option that effects a bunch of other settings. The same is true of -ffp-model. In Driver/ToolChains/Clang.cpp we try to unravel things to produce diagnostics when certain options change, but we're inconsistent and sometimes, such as in this case, just wrong.

A few other combinations to consider:

-ffast-math -ffp-contract=on
No diagnostic

-ffp-model=fast -ffp-contract=on
warning: overriding '-ffp-model=fast' option with '-ffp-contract=on' [-Woverriding-t-option]

-ffp-contract=on -ffp-model=fast
No diagnostic

-ffp-model=precise -ffp-contract=fast
No diagnostic

-fno-fast-math -ffast-math
No diagnostic

What we're trying to do is let the user know about cases when they've used options that changed the fp-contract settings in ways they may not have noticed. I think I agree that it might be better not to issue any diagnostic.

[AaronBallman]

I think I agree that it might be better not to issue any diagnostic.

I'm not certain I agree. If the user has given conflicting options that we cannot honor, that seems like a situation we should warn on (we do for quite a few other command line flags that are mutually exclusive to one another). What benefit does the user receive by being silent rather than correcting the diagnostic behavior?

[efriedma-quic]

Overriding flags (i.e. "the last flag wins") is a feature: it allows you to have a set of default flags for your build, and change them for specific files/libraries.

Maybe it makes sense to allow users to request warnings for redundant/overridden flags, but I don't think we'd want to turn that on by default.

[AaronBallman]

Okay, I definitely agree that last flag wins makes sense for -ffast-math -fno-fast-math`. We use that pattern all over the place, and we should continue to support it.

But I think -ffp-model=fast -ffp-contract=on (and similar) is an entirely different beast. It seems quite likely for users to write this command line without any idea that there's a "last flag wins" situation here because there's no obvious relationship between the flags. And we do diagnose that kind of situation in the driver by default.

Do you think that you think users will expect a last-flag-wins behavior for those scenarios as well?

[efriedma-quic]

I think users will still expect last-flag-wins... but they might get confused about exactly what it means for a flag to "win".

If we can offer the user some way to fix the flags "locally", it might be okay. e.g. "warning: -ffp-contract=on overrides flag -ffp-model=fast. note: replace -ffp-contract=on with -ffp-model=strict -ffast-math." Not really happy with the exact language there, but you get the idea. In that case, it might be okay... but that seems complicated to implement in a way that will actually help users.

[AaronBallman]

I think users will still expect last-flag-wins... but they might get confused about exactly what it means for a flag to "win".

That's basically my concern -- the flags seem unrelated, so "winning" is very unclear, and being silent means users in this situation will most likely have no idea there was a winner and a loser involved.

If we can offer the user some way to fix the flags "locally", it might be okay. e.g. "warning: -ffp-contract=on overrides flag -ffp-model=fast. note: replace -ffp-contract=on with -ffp-model=strict -ffast-math." Not really happy with the exact language there, but you get the idea. In that case, it might be okay... but that seems complicated to implement in a way that will actually help users.

That's effectively the outcome I was hoping for, but I'd even be fine if the diagnostic was warning: '-ffp-contract-on' overrides '-ffp-model=fast'; the latter option is ignored It's not as helpful as "here's how to correct this", but it's at least somewhat helpful in that it tells users something potentially unexpected has happened. I'd have it under its own warning group so people can disable the diagnostic as needed. (My intuition is that this is sufficiently important that it should be on-by-default -- if we find evidence to suggest that this is actually a fairly common situation and users don't appear to be confused by it in practice, then I agree we don't need a diagnostic at all.)

[zahiraam]

That's basically my concern -- the flags seem unrelated, so "winning" is very unclear, and being silent means users in this >>situation will most likely have no idea there was a winner and a loser involved.
Strongly agree with this.

That's effectively the outcome I was hoping for, but I'd even be fine if the diagnostic was warning: '-ffp-contract-on' overrides >>'-ffp-model=fast'; the latter option is ignored It's not as helpful as "here's how to correct this", but it's at least somewhat
I think a warning saying this would be good. Trying to offer alternatives to the user might not be that easy to do and there might be multiple ones to offer.

[andykaylor]

I was trying to find some precedent for handling this in other compilers. GCC was the original source of a lot of these partially conflicting floating point options, but it doesn't offer any such warnings. I guess the idea is that users understand that things like -ffast-math and -funsafe-math-optimizations are umbrella settings and interact in expected ways with individual settings like -fassociative-math.

I was sure I had seen ICC report overriding behavior with its fp-model option, but I can't seem to find a combination now that reports it. There are a lot of other cases where ICC prints warnings about conflicting options, mostly related to pgo, but I couldn't find a case where it did with the floating point options.

Perhaps a bigger problem with regard to -fp-contract is that in GCC the -f[no-]fast-math option doesn't control fp-contract behavior. I guess it's a bit late for us to start changing that now. We've been turning fp-contract on with -ffast-math for five years, and I think it's the right thing to do. What's less obvious is that -fno-fast-math attempts to set it back to its default state. So we get this (except with HIP and CUDA):

'-ffp-contract=fast -fno-fast-math' --> fp-contract = on
'-fno-fast-math -ffp-contract=fast' --> fp-contract = fast
'-ffp-contract=off -fno-fast-math' --> fp-contract = off
'-ffp-contract=off -ffast-math -fno-fast-math' --> fp-contract = on
'-ffast-math -ffp-contract=off -fno-fast-math' --> fp-contract = off
'-ffast-math -ffp-contract=fast -fno-fast-math' --> fp-contract = on

We don't have problems like this with options like -freciprocal-math because -fno-fast-math turns it off. In the fact of fp-contract, if FPContract is "fast" RenderFloatingPointOptions sets it to "on" and if it's either "on" or "off" it leaves it alone. This is kind of assuming that if it's "fast" that is probably because of a previous -ffast-math option, but that's not a very good assumption.

We could track whether there was a previous explicit -ffp-contract option, but that would still depend on option ordering if both -ffp-contract and -ffast-math were used (for instance, should "-ffp-contract=fast -ffast-math -fno-fast-math" behave the same as "-ffast-math -ffp-contract=fast -fno-fast-math"). The only entirely consistent thing I can think of would be to have -fno-fast-math always set fp-contract to "off".

In any case, I think it may be best to omit the warnings here, at least by default, and instead make our handling more predictable and consistent.

[AaronBallman]

I was trying to find some precedent for handling this in other compilers. GCC was the original source of a lot of these partially conflicting floating point options, but it doesn't offer any such warnings. I guess the idea is that users understand that things like -ffast-math and -funsafe-math-optimizations are umbrella settings and interact in expected ways with individual settings like -fassociative-math.

I would not hold GCC up as a particularly good practice in this case; GCC has traditionally been silent on a lot of things Clang gives diagnostics for.

I was sure I had seen ICC report overriding behavior with its fp-model option, but I can't seem to find a combination now that reports it. There are a lot of other cases where ICC prints warnings about conflicting options, mostly related to pgo, but I couldn't find a case where it did with the floating point options.

Perhaps a bigger problem with regard to -fp-contract is that in GCC the -f[no-]fast-math option doesn't control fp-contract behavior. I guess it's a bit late for us to start changing that now. We've been turning fp-contract on with -ffast-math for five years, and I think it's the right thing to do. What's less obvious is that -fno-fast-math attempts to set it back to its default state. So we get this (except with HIP and CUDA):

'-ffp-contract=fast -fno-fast-math' --> fp-contract = on '-fno-fast-math -ffp-contract=fast' --> fp-contract = fast '-ffp-contract=off -fno-fast-math' --> fp-contract = off '-ffp-contract=off -ffast-math -fno-fast-math' --> fp-contract = on '-ffast-math -ffp-contract=off -fno-fast-math' --> fp-contract = off '-ffast-math -ffp-contract=fast -fno-fast-math' --> fp-contract = on

We don't have problems like this with options like -freciprocal-math because -fno-fast-math turns it off. In the fact of fp-contract, if FPContract is "fast" RenderFloatingPointOptions sets it to "on" and if it's either "on" or "off" it leaves it alone. This is kind of assuming that if it's "fast" that is probably because of a previous -ffast-math option, but that's not a very good assumption.

We could track whether there was a previous explicit -ffp-contract option, but that would still depend on option ordering if both -ffp-contract and -ffast-math were used (for instance, should "-ffp-contract=fast -ffast-math -fno-fast-math" behave the same as "-ffast-math -ffp-contract=fast -fno-fast-math"). The only entirely consistent thing I can think of would be to have -fno-fast-math always set fp-contract to "off".

In any case, I think it may be best to omit the warnings here, at least by default, and instead make our handling more predictable and consistent.

Predictable and consistent option handling would be good, but the above explanation is what I'm pretty strongly in favor of diagnosing if we're not able to change option behavior to make them predictable and consistent. I don't think it's reasonable to expect users to have that much detailed knowledge about how these options interact in these complex scenarios, especially given that Clang and GCC differ in their treatment of them.

At the very least, we know that the -ffast-math -fno-fast-math behavior is wrong and needs to be addressed. So my recommendation is to disable the diagnostics temporarily but commit to adding some more palatable diagnostics or to modifying the option behavior to make them more predictable for users in short order.