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

Mon Nov 7 11:39:54 PST 2022

zahiraam added a comment.

In D136786#3909043 <https://reviews.llvm.org/D136786#3909043>, @zahiraam wrote:

> In D136786#3907235 <https://reviews.llvm.org/D136786#3907235>, @michele.scandale wrote:
>
>> In D136786#3903646 <https://reviews.llvm.org/D136786#3903646>, @zahiraam wrote:
>>
>>> The changes in this patch look good to me. 
>>> @michele.scandale please make sure not to drop the driver changes that we agreed upon in this patch. Thanks.
>>
>> I started looking at the change needed to have `unsafe-math => fp-contract=fast`.
>>
>> If I look how `-ffast-math` behave today, I see that in the driver code `-ffast-math` changes the state for `FPContract` (https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L3032), but then the condition for which `-ffast-math` is forwarded to the CC1 (https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L3149) doesn't consider `FPContract`. This seems related to the fact that GCC does emit `__FAST_MATH__` even if the contraction mode is not fast.
>> From a quick look the `FastMath` language options is used mainly to guard some macro definition and a codegen path for complex floating point values, so this seems ok in practice.
>>
>> It seems intended that the semantic of `-ffast-math` for the CC1 is different than the semantic of `-ffast-math` for the compiler driver. Based on this, I'd expect a similar solution for `-funsafe-math-optimizations`, i.e. `-funsafe-math-optimizations => -ffp-contract=fast` only at the compiler driver level.  Does this sound good?
>>
>> If so, then the driver change for the `-funsafe-math-optimizations -> -ffp-contract=fast` could be done separately without affecting the code here.
>>
>>> I talked to @andrew.w.kaylor offline: I was thinking that it might be necessary to make the two driver changes we talked about, before merging this patch. But if the tests pass then I think it's okay to implement the driver changes in an upcoming patch.
>>
>> In this patch the only suboptimal test change is the change to `clang/test/CodeGenOpenCL/relaxed-fpmath.cl`, where despite the presence of `-cl-unsafe-math-optimizations` the `"unsafe-fp-math"="true"` function attributes is not generated due to the contraction mode not being fast.
>> My understanding is that `-cl-fast-relaxed-math` should be equivalent to `-ffast-math`, and `-cl-unsafe-math-optimizations` should be equivalent to `-funsafe-math-optimizations` from the user perspective.
>>
>> From what I see the `-cl-*` options are simply forwarded as-is to the CC1, and this seems to be the desired behavior for the compiler driver w.r.t. OpenCL specific options. From what I see https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L3803 implements `-cl-fast-relaxed-math => -ffast-math`, but this solution doesn't play well if on the same command line you also have `-ffp-contract=VAL` as the relative order of the options is not taken into account. I'd expect a similar change for the `-cl-unsafe-math-optimizations` case.
>>
>> I can either put a `TODO` comment in the `clang/test/CodeGenOpenCL/relaxed-fpmath.cl` test, and make the changes with the driver changes for `-funsafe-math-optimizations`, otherwise the change for the `-cl-unsafe-math-optimizations` options needs to be done in this patch.
>>
>> Any preference?
>
> That's exactly what I was worried about.
> I think it would be best to create another patch to make the 'funsafe-math-optimization' => FpContract=fast and make sure we have coherence with 'ffast-math' and the cl options, then check in this patch once that's done.

@andrew.w.kaylor , @michele.scandale I created a draft patch https://reviews.llvm.org/D137578 that fixes the 2 issues mentioned above. Let me know if the entry is visible to you (it's a private entry for now)? Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786