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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:3031
 
+The ``#pragma float_control`` pragma allows floating-point precision and
+floating-point exception behavior to be specified
----------------
Floating-point precision refers to the number of bits in mantissa, here the term `precise floating-point semantics` looks more appropriate.


================
Comment at: clang/docs/LanguageExtensions.rst:3056
+    a = b[i] * c[i] + e;
+  }
 Specifying an attribute for multiple declarations (#pragma clang attribute)
----------------
Blank line is needed after the end of paragraph.


================
Comment at: clang/include/clang/AST/Decl.h:180
+/// Support for `#pragma float_control` line.
+class PragmaFloatControlDecl {
+
----------------
What is the purpose of this class? Do you plan to extend it? If you need to restrict the scope of constants like `FC_Unknown` it is better to use scoped enumerations.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1279
   "compound statement">;
+def err_pragma_fc_scope : Error<
+  "'#pragma float_control' can only appear at file scope or at the start of a "
----------------
We already have several pragmas that require the same restriction (`clang fp`, `STDC FENV_ACCESS`) and will add some more (`STDC FENV_ROUND`), probably it makes sense to use generic message and supply pragma name as argument?


================
Comment at: clang/include/clang/Sema/Sema.h:543
 
+  typedef struct {
+    bool HasPragma;
----------------
Why typedef, not simply `struct FpPragmaStateType`?
Usually we use C++ style of struct declarations.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:111
 // the corresponding enum in the IR.
-static llvm::fp::RoundingMode ToConstrainedRoundingMD(
+llvm::fp::RoundingMode clang::ToConstrainedRoundingMD(
   LangOptions::FPRoundingModeKind Kind) {
----------------
Is `clang::` necessary here? The file already has `using namespace clang;`.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:126
 // the corresponding enum in the IR.
-static llvm::fp::ExceptionBehavior ToConstrainedExceptMD(
+llvm::fp::ExceptionBehavior clang::ToConstrainedExceptMD(
   LangOptions::FPExceptionModeKind Kind) {
----------------
Ditto.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:2522
+  IdentifierInfo *II = Tok.getIdentifierInfo();
+  PragmaFloatControlDecl::FloatControlKind Kind =
+    llvm::StringSwitch<PragmaFloatControlDecl::FloatControlKind>(
----------------
Does such treatment allow a pragma like:

    #pragma #pragma float_control(except, on), push

The comment to `PragmaFloatControlHandler::HandlePragma` says it is valid.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:430
+    break;
+  case PragmaFloatControlDecl::FC_Push:
+  case PragmaFloatControlDecl::FC_Pop:
----------------
`push` cannot be combined with `precise`?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13129
     if (FunctionDecl *F = dyn_cast<FunctionDecl>(CurContext)) {
+      // If the expression occurs inside an internal global_var_init_function
+      // then the FunctionDecl is not availble
----------------
The standard says that static initializers execute in default FP mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841