[PATCH] D77545: Represent FP options in AST by special Expression node

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> ! In D77545#1973765 <https://reviews.llvm.org/D77545#1973765>, @sepavloff wrote:
> 
>> ! In D77545#1973169 <https://reviews.llvm.org/D77545#1973169>, @rjmccall wrote:
>>  all of which will just do the wrong thing if they don't preserve and pass down the right information.  It's extremely error-prone, and I don't see a way to make it not error-prone.
> 
> Actually support of FP environment is very similar to scope mechanism. It is changed *only* at compound statement entry or exit. We already have many RAII objects that facilitate this maintenance

Okay, so you're proposing that we maintain global state in the ASTContext for what pragmas are in effect, managed by RAII objects, rather than passing the current pragma state in as an argument.  I agree that this is relatively straightforward for clients to support if they're just performing a recursive walk.  However, it is extremely susceptible to bugs where the current context is incorrectly applied to expressions written in a different context.

For example, in this code:

  const float global = 2.22 + 3.33;
  
  #pragma STDV FENV_ROUND <direction>
  float getGlobal() { return global; }

The code-generation of `getGlobal` will attempt to constant-evaluate the initializer of `global` so that it can just return a constant instead of emitting a load from a global.  It is important for correctness that this constant-evaluation occur in the default FP environment, not the active FP environment at the point of use.  Presumably you are not proposing that `global`'s initializer is a `PragmaExpr`, since it's in the default environment.  Perhaps we can recognize by the *absence* of a `PragmaExpr` that it's meant to use the default environment?  It's not clear at what level we would trigger this check, however.  Is it now semantically important that we call a method like `VarDecl::evaluateValue()` rather than `Expr::EvaluateAsFloat(...)`?  What happens with variables in local scope?  A local variable could be from a scope outside the current pragma, so presumably we need to introduce `PragmaExpr`s on the initializers of all variables within pragmas.

This sort of problem is why having the pragma state be an argument rather than global state is so much more attractive.  And making it an argument is much more disruptive for clients, who must now do their own tracking, and recreate the state appropriately when calling outside the context.

Storing the pragma state in the expressions avoids these problems much more cleanly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77545