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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits