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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D123952#3488638 <https://reviews.llvm.org/D123952#3488638>, @aaron.ballman wrote:

> Thanks for working on this! One thing that's not clear to me is what bugs this is solving -- the test coverage only shows a change to textual AST dumping behavior. Is this otherwise expected to be an NFC change, or should there be some codegen tests that show a difference in behavior?

Thanks for feedback!
This change is a prerequisite for implementation of pragma STDC FENV_ROUND in D125625 <https://reviews.llvm.org/D125625>. FP options stored in CompoundStmt are used to set up proper rounding mode on entry to compound statement.



================
Comment at: clang/include/clang/AST/Stmt.h:134
+    /// floating-point features.
+    unsigned HasFPFeatures : 1;
+
----------------
aaron.ballman wrote:
> I don't think this is a bad approach, but it does further reduce the number of statements we support in a compound statement.
> 
> There's a part of me that wonders if the approach is to introduce a new AST node for a stateful compound statement; I suspect if we dig around, we'll find other pragmas that want to behave similar to floating-point feature pragmas (the idea of a pragma scoped to a block is not really new). Having something more general means we're less likely to keep stealing bits here.
Possible solution is presented in D125635.
Spending a bit for state variable reduces number of bits for NumStmts to 23 bits (about 8M statements). And yes, other pragmas (or anything else) can require allocation of additional bits. As the number of statements is potentially large, it would be better to keep in an integer rather than bit-field.


================
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:
> 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.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:2372-2376
+void TextNodeDumper::VisitCompoundStmt(const CompoundStmt *S) {
+  VisitStmt(S);
+  if (S->hasStoredFPFeatures())
+    printFPOptions(S->getStoredFPFeatures());
+}
----------------
aaron.ballman wrote:
> Should we have a similar change for the JSON node dumper?
Implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123952