mibintc marked 6 inline comments as done. mibintc added a comment. In D72841#1842772 <https://reviews.llvm.org/D72841#1842772>, @andrew.w.kaylor wrote:
> It's not clear to me from reading this how the "precise" control is going to > work with relation to the fast math flags. I don't think MSVC allows the > granularity of control over these flags that we have in clang, so maybe they > don't have this problem. You're right, MSVC only allows the pragma at file scope. > Consider this code: https://godbolt.org/z/mHiLCm > > With the options "-ffp-model=precise -fno-honor-infinities -fno-honor-nans" > the math operations here are generated with the "nnan ninf contract" flags. > That's correct. What will happen when I use the pragma to turn precise off? > Does it enable all fast math flags? Will the subsequent "pop" leave the "ninf > nnan" fast math flags enabled? This patch doesn't have support for turning precise off, that's a bug, I will revisit. This is my plan for how to handle enabling/disabling fast math: IRBuilder.h has settings FMF, and also supplies clearFastMathFlags, setFastMathFlags(flags) and FastMathFlagGuard. When the expression is walked that alters the FMF, use FastMathFlagGuard to save the current state of FMF, modify the settings using the clear or set functions, walk the expression. After the expression is walked, the FMF settings will be restored to previous state by the FastMathFlagGuard destructor. > As I said, I don't think you can get into this situation with MSVC. I believe > that icc will go into full fast math mode with the "precise, off, push" > pragma but will go back to "nnan ninf contract" mode with the pop. At least, > that's what the design docs say. I haven't looked at the code to verify this. > It seems like the correct behavior in any case. I think the clang FPOptions > needs individual entries for all of the fast math flags to handle this case. Thanks I'll investigate this and add test cases. I think possibly since IRBuilder has the FMF like I described above it might work with current support. Is there currently a way to modify nan and inf at the source level or only by compiler option? BTW I've been asked to implement a pragma to control fp "reassoc" at the source level. I'm planning to do that after this patch is complete. ================ Comment at: clang/docs/LanguageExtensions.rst:3042 +by the pragma behaves as though the command-line option ``ffp-model=precise`` +is enabled. That is, fast-math is disabled and fp-contract=on (fused +multiple add) is enabled. ---------------- andrew.w.kaylor wrote: > Re "fp-contraction=on": I agree that this is what it should do, but I don't > think that's what fp-model=precise currently does. I think it's setting > fp-contraction=fast. Oh, I looked back at the patch for -ffp-model and precise is documented to set ffp-contract=fast. Not sure why I thought that was right. I'll have to redo it. ================ Comment at: clang/docs/LanguageExtensions.rst:3050 + +The full syntax this pragma supports is ``float_control(except|precise, on|off, [push])`` and ``float_control(push|pop)``. The ``push`` and ``pop`` forms can only occur at file scope. + ---------------- andrew.w.kaylor wrote: > andrew.w.kaylor wrote: > > Looks like you need a line break here. > Are the precise and except stacks independent? No, the stack that tracks the float control pragma settings is a pair, roughly (IsPreciseEnabled, IsExceptEnabled) ================ Comment at: clang/include/clang/Basic/LangOptions.h:363 + exceptions(LangOptions::FPE_Ignore), + fp_precise(false) {} ---------------- andrew.w.kaylor wrote: > It seems like fp_precise describes too many things to be a single option. > Even within this set of options it overlaps with fp_contract. I see your point. I wanted it to reflect the current pragma setting that's why I kept it intact. I'll rethink this. ================ Comment at: clang/test/CodeGen/fp-floatcontrol-stack.cpp:124 +#if DEFAULT +//CHECK-DDEFAULT: fmul float +//CHECK-DDEFAULT: fadd float ---------------- andrew.w.kaylor wrote: > Are there also fast-math flags set here? If not, why not? that's a bug. thanks ================ Comment at: clang/test/CodeGen/fp-floatcontrol-stack.cpp:2 +// RUN: %clang_cc1 -DDEFAULT=1 -emit-llvm -o - %s | FileCheck --check-prefix=CHECK-DDEFAULT %s +// RUN: %clang_cc1 -DEBSTRICT=1 -ffp-exception-behavior=strict -emit-llvm -o - %s | FileCheck --check-prefix=CHECK-DEBSTRICT %s + ---------------- andrew.w.kaylor wrote: > Can you add run lines for -ffast-math and (separately) "-fno-honor-nans > -fno-honor-infinities"? OK. i'll add pragma's to set precise off too. ================ Comment at: clang/test/CodeGen/fp-floatcontrol-stack.cpp:17 +// for exception behavior and rounding mode. +//CHECK-DEBSTRICT: llvm.experimental.constrained.fmul{{.*}}tonearest{{.*}}strict +#endif ---------------- andrew.w.kaylor wrote: > There should be a constrained fadd here also, right? yes there's a constrained add following. i can add that pattern into the file check. ================ Comment at: clang/test/CodeGen/fp-floatcontrol-stack.cpp:52 +#if EBSTRICT +//CHECK-DEBSTRICT: llvm.experimental.constrained.fmul{{.*}}tonearest{{.*}}ignore +#endif ---------------- andrew.w.kaylor wrote: > Why is the constrained intrinsic generated in this case? If we've got both > constraints set to the defaults at the file scope I would have expected that > to turn off the constrained mode. The "run" line in this case uses ffp-exception-behavior=struct; i was trying to address https://bugs.llvm.org/show_bug.cgi?id=44571 by checking the command line options to see if strict was enabled. That's why constrained intrinsics are enabled. Evidently that's incorrect. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72841/new/ https://reviews.llvm.org/D72841 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
