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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.h:747
+  /// Return difference with the given option set.
+  FPOptionsOverride diffWith(const FPOptions &Base);
+
----------------
rjmccall wrote:
> Can you make direction more obvious in the method name?  `diffFrom` would make it clear that the result, applied to the base, yields `this`.
I renamed this method to `getChanges`. I am not sure it becomes better. May be `getChangesFrom` or `getDiffFrom`? Any suggestion is welcome.


================
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;
----------------
rjmccall wrote:
> 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.
It makes sense. Renamed to `InitialFPFeatures`.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:216
+    LangOptions::RoundingMode RM = FPO.getConstRoundingModeOverride();
+    if (RM != llvm::RoundingMode::Dynamic) {
+      Indent() << "#pragma STDC FENV_ROUND ";
----------------
aaron.ballman wrote:
> Why is this not handled as well and print out `FE_DYNAMIC`?
Initially it was used to prevent extra `FENV_ROUND FE_DYNAMIC` pragma in the case when only `FENV_ACCESS` was specified. However after landing D126364 there is no need for that.  Now `FE_DYNAMIC` is treated similar to other values.


================
Comment at: clang/lib/Basic/LangOptions.cpp:214
+    OverrideMask |= NAME##Mask;
+#include "clang/Basic/FPOptions.def"
+  return FPOptionsOverride(*this, OverrideMask);
----------------
rjmccall wrote:
> 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.
Implemented slow and inline versions. The later is defined after definition of `FPOptionsOverride` in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123952