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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
