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

Zahira Ammarguellat via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 4 12:45:32 PDT 2022


zahiraam added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786



More information about the cfe-commits mailing list