[PATCH] D93769: [clang] Add support for option -ffp-eval-method and extend #pragma float_control similarly

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:3399
+by the pragma behaves as though the command-line option
+``-ffp-eval-method=source`` is enabled, returning floating point evaluation
+method to the default setting.
----------------



================
Comment at: clang/docs/UsersManual.rst:1486
+   Valid values are: ``source``, ``double``, and ``extended``.
+   The default value is target specific, typically ``source``.  Details:
+
----------------



================
Comment at: clang/include/clang/Basic/LangOptions.h:226
 
+  /// Possible float expression evaluation method choies.
+  enum FPEvalMethodKind {
----------------



================
Comment at: clang/include/clang/Lex/Preprocessor.h:181
   IdentifierInfo *Ident__is_target_environment;    // __is_target_environment
+  IdentifierInfo *Ident__FLT_EVAL_METHOD__;        // __FLT_EVAL_METHOD__
 
----------------
Otherwise, when `LexExpandSpecialBuiltins` is `false`, I think this winds up being an uninitialized pointer value.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2803
 
+    // Validate and pass through -ffp-float-precision option.
+    case options::OPT_ffp_eval_method_EQ: {
----------------



================
Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1743
         });
-  } else if (II == Ident__has_include ||
-             II == Ident__has_include_next) {
+  } else if (II == Ident__has_include || II == Ident__has_include_next) {
     // The argument to these two builtins should be a parenthesized
----------------
Unrelated formatting change?


================
Comment at: clang/lib/Sema/Sema.cpp:216
+    // Use setting from TargetInfo.
+    PP.setCurrentFltEvalMethod(static_cast<LangOptions::FPEvalMethodKind>(
+        ctxt.getTargetInfo().getFloatEvalMethod()));
----------------
I don't think this cast is needed, is it? (It might make sense for the `getFloatEvalMethod()` return type to be `int` rather than `unisnged` though, given that we support negative values for implementation-defined semantics?)


================
Comment at: clang/lib/Sema/Sema.cpp:220
+    // Set initial value of __FLT_EVAL_METHOD__ from the command line.
+    PP.setCurrentFltEvalMethod(getLangOpts().getFPEvalMethod());
 }
----------------
Heh, so we have:

`setCurrentFltEvalMethod`
`getFloatEvalMethod`
`getFPEvalMethod`

I think we should pick a consistent spelling for `float evaluation method` and go with it. I don't know that work needs to happen as part of this patch (it can be a follow-up), but I think it'd help with readability.


================
Comment at: clang/test/CodeGen/fp-floatcontrol-pragma.cpp:240
+
+float mySub(float x, float y) {
+  // CHECK: define {{.*}}float {{.*}}mySub{{.*}}
----------------
Can we also get some tests that show how this behaves with the less common floating-point types like float16, bfloat16, float128, half?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93769