[PATCH] D136786: Fix `unsafe-fp-math` attribute emission.

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In D136786#3896708 <https://reviews.llvm.org/D136786#3896708>, @michele.scandale wrote:

> In D136786#3896341 <https://reviews.llvm.org/D136786#3896341>, @zahiraam wrote:
>
>>> I'm going to ignore fast-math right now, because I think the current handling is mostly OK, and this review is about unsafe-fp-math. The unsafe-fp-math case is a little simpler in concept, but I think we've got more problems with it.
>>>
>>> Another fundamental principle I'd like to declare here is that LangOpts.UnsafeFPMath, TargetOptions.UnsafeFPMath, and the "unsafe-fp-math" function attribute should all mean exactly the same thing. The function attribute has a different scope, so it might not always have the same value as the other two, but it should at least mean the same thing.
>>>
>>> My understanding is this:
>>>
>>>   unsafe-fp-math = exception_behavior(ignore) +
>>>                    fenv_access(off) +
>>>                    no_signed_zeros(on) +
>>>                    allow_reciprocal(on) +
>>>                    allow_approximate_fns(on) +
>>>                    allow_reassociation(on) +
>>>                    fp_contract(fast)
>>>
>>> The first two of those settings are the default behavior. The others can be turned on by individual command line options. If all of those semantic modes are in the states above, then we are in "unsafe-fp-math" mode. That's my proposal.
>>
>> Agree. This is not currently the case.  -funsafe-math-optimizations should set the FPContract=fast the same way it's done for -ffast-math. I think it makes sense to fix this in this patch. @michele.scandale WDYT?
>
> (*) I understand that Clang has a different definition of `-ffast-math` compared to the GCC one (i.e. in Clang `-ffast-math` implies `-ffp-contract=fast`), and that this can be considered an indication that there isn't a strong desire to be compatible with GCC. If that's the generally accepted direction, I won't oppose it.

This is not new. It's already done this way in the current clang. So, it looks like the decision has been taken already.

> Given that we all agreed that we should be using only low level options inside the compiler, I would keep changes to the compiler driver separate from this IR code generation change, and the two should be completely independent.

Okay.

>>> There are a couple of things we need to talk about here:
>>>
>>> 1. Currently, the Driver/ToolChains/Clang.cpp RenderFloatingPointOptions() function is looking for !MathErrno before deciding to pass "-funsafe-math-optimizations" to the front end, so LangOpts.UnsafeFPMath will not be set unless math-errno support is off. However, the RenderFloatingPointOptions() handling of -funsafe-math-optimizations does not change the MathErrno setting. Clearly, there's a bug here one way or the other. In GCC -funsafe-math-optimizations does not affect the math errno handling, so I think that's the correct behavior.
>>
>> Yep!   This can be fixed in an upcoming patch. @michele.scandale Do you want to do it? If not, I can.
>
> I can prepare a patch for that, and if there is a general consensus about (*) then we can have a single change that fixes the compiler driver code.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136786/new/

https://reviews.llvm.org/D136786