[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.h:747
+  /// Return difference with the given option set.
+  FPOptionsOverride diffWith(const FPOptions &Base);
+
----------------
Can you make direction more obvious in the method name?  `diffFrom` would make it clear that the result, applied to the base, yields `this`.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:77-78
 
-  CompoundScopeInfo(bool IsStmtExpr) : IsStmtExpr(IsStmtExpr) {}
+  /// FP options at the beginning of the compound statement, prior to
+  /// any pragma.
+  FPOptions FPFeatures;
----------------
aaron.ballman wrote:
> sepavloff wrote:
> > aaron.ballman wrote:
> > > So these are the initial FPOptions inherited by the scope from its surrounding context? And it's never updated by a pragma?
> > Yes, this FPOption is used to calculate the effect of pragmas in the compound statement as a difference with FPOptions stored in Sema, so that CompoundStmt keeps only FP features that are changed in it.
> It might make sense to rename this to `OriginalFPFeatures` or `InitialFPFeatures` or something like that, to make it more clear that this field is not updated as we process the rest of the compound statement. WDYT?
Yeah, I agree; this name makes it sound like the current, live set of features, even if the comment explains that it isn't.


================
Comment at: clang/lib/AST/Stmt.cpp:370-371
   setStmts(Stmts);
+  if (hasStoredFPFeatures())
+    setStoredFPFeatures(FPFeatures);
 }
----------------
aaron.ballman wrote:
> There's a part of me that wonders if it's a slightly more clear design to have `setStoredFPFeatures()` set `CompoundStmtBits.HasFPFeatures` to true as needed rather than requiring the caller to do this two-step initialization process. WDYT?
`setStoredFPFeatures` is only otherwise used in deserialization, and the node has to be allocated properly to support it.  I think this is the right approach.


================
Comment at: clang/lib/Basic/LangOptions.cpp:214
+    OverrideMask |= NAME##Mask;
+#include "clang/Basic/FPOptions.def"
+  return FPOptionsOverride(*this, OverrideMask);
----------------
sepavloff wrote:
> rjmccall wrote:
> > Hmm.  If we can assume that all of the options are single bits (which this is doing), then this algorithm is basically `FPOptions::storage_type OverrideMask = (Value ^ Base.Value);`, which makes this cheap enough to be defined inline in the header.
> `FPContractMode`, `ConstRoundingMode`, `SpecifiedExceptionMode` and `FPEvalMethod` occupy more than one bit: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FPOptions.def.
Oh, I see.  I wasn't reasoning correctly about how this is a mask and not a value.

In non-LTO builds, we'll have to actually do this call when leaving every scope.  It's a minor overhead, but it'd be nice to avoid.  Please do the fast path in an inline implementation and then put this slow path out of line, in a private method named `diffFromSlow` or something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123952