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

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:3055
+same spelling and syntax.  For pragmas specified at file scope, a stack
+is supported so that the pragma float_control settings can be pushed or popped.
+
----------------
`pragma float_control` should be in backticks.

Throughout this documentation, when referring to command-line options, please spell them the way they're actually spelled on the command line, i.e. with a dash.


================
Comment at: clang/include/clang/AST/Stmt.h:1104
+    static_assert(sizeof(*this) <= 16,
                   "changing bitfields changed sizeof(Stmt)");
     static_assert(sizeof(*this) % alignof(void *) == 0,
----------------
What's happening here is exactly what this assertion is supposed to prevent.   If you need more bits in one of these classes (I assume it's `CXXOperatorCallExpr`), you need to either make a field in the actual class or investigate more arcane mechanisms like  trailing storage to reduce the normal impact.  The latter is probably unnecessary for `CXXOperatorCallExpr`.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1138
+def err_pragma_float_control_unknown_kind : Error<
+  "unknown kind of pragma float_control">;
 // - #pragma pointers_to_members
----------------
Maybe "operation" would be a better user-facing name than "kind"?   Also, this diagnostic is more specific but less helpful than the diagnostic just above.


================
Comment at: clang/include/clang/Basic/LangOptions.def:196
+COMPATIBLE_LANGOPT(AllowRecip        , 1, 0, "Permit Floating Point reciprocal")
+COMPATIBLE_LANGOPT(ApproxFunc        , 1, 0, "Permit Floating Point approximation")
 
----------------
Please align the commas.

Would it make more sense to just store an `FPOptions` in `LangOptions` instead of breaking all of the bits down separately?

We may need to reconsider at some point whether any of these are really "compatible" language options.  Headers can contain inline code, and we shouldn't compile that incorrectly just because we reused a module we built under different language settings.  Although... maybe we can figure out a way to store just the ways that an expression's context overrides the default semantics and then merge those semantics into the default set for the translation unit; that would make them actually compatible.  Of course, it would also require more bits in expressions where it matters, and you might need to investigate trailing storage at that point.


================
Comment at: clang/include/clang/Basic/LangOptions.h:468
+  bool allowReciprocal() const { return allow_reciprocal; }
+  bool approxFunc() const      { return approx_func; }
+
----------------
Somewhere in this type, it should be obvious where I can go in order to understand what any of these flags means precisely.  Ideally that would be reinforced by the method names, instead of using non-term-of-art abbreviations like "reassoc".


================
Comment at: clang/include/clang/Basic/LangOptions.h:517
+    approx_func = ((I>>13) & 1);
   }
 
----------------
The more conventional method names here would an instance method called something like `getAsOpaqueInt` and then a static method called something like `getFromOpaqueInt`.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:462
+      return StmtVisitor<ScalarExprEmitter, Value*>::Visit(E);
+    }
     return StmtVisitor<ScalarExprEmitter, Value*>::Visit(E);
----------------
You can override `VisitBinOp` and just do this in that case.  But why does it need to be done at this level at all, setting global state on the builder for all emissions, instead of in the leaves where we know we're emitting floating-point operations?  This is adding a lot of overhead in some of the most commonly-exercised code paths in IRGen, but building FP expressions is relatively uncommon.  I would definitely prefer a little bit of repetitive code over burdening the common case this much.  It might also be nice to figure out when this is unnecessary.

Also, please extract a function to make FastMathFlags from FPOptions; you'll need it elsewhere, e.g. in CGExprComplex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841