[PATCH] D72841: [RFC] Add support for pragma float_control, to control precision and exception behavior at the source level

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

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.

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?

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.



================
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.
----------------
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.


================
Comment at: clang/docs/LanguageExtensions.rst:3043
+is enabled.  That is, fast-math is disabled and fp-contract=on (fused
+multiple add) is enabled.
+
----------------
s/multiple/multiply


================
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.
+
----------------
Looks like you need a line break here.


================
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:
> Looks like you need a line break here.
Are the precise and except stacks independent?


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1112
   InGroup<IgnoredPragmas>;
 // - #pragma fp_contract
+def err_pragma_file_or_compound_scope : Error<
----------------
This comment is wrong after your change.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1135
+def err_pragma_float_control_malformed : Error<
+  "pragma float_control is malformed; it requires one or two comma-separated "
+  "arguments">;
----------------
This isn't quite accurate. The pop case has no comma-separated arguments. It might be better to print the full syntax here if that's feasible.


================
Comment at: clang/include/clang/Basic/LangOptions.h:363
+                exceptions(LangOptions::FPE_Ignore),
+                fp_precise(false)
         {}
----------------
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.


================
Comment at: clang/test/CodeGen/fp-floatcontrol-stack.cpp:124
+#if DEFAULT
+//CHECK-DDEFAULT: fmul float
+//CHECK-DDEFAULT: fadd float
----------------
Are there also fast-math flags set here? If not, why not?


================
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
+
----------------
Can you add run lines for -ffast-math and (separately) "-fno-honor-nans -fno-honor-infinities"?


================
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
----------------
There should be a constrained fadd here also, right?


================
Comment at: clang/test/CodeGen/fp-floatcontrol-stack.cpp:52
+#if EBSTRICT
+//CHECK-DEBSTRICT: llvm.experimental.constrained.fmul{{.*}}tonearest{{.*}}ignore
+#endif
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841