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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D77545#1974509 <https://reviews.llvm.org/D77545#1974509>, @rjmccall wrote:

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


As you correctly noted, putting `PragmaExpr` around initializer expression even in default environment solves all these problems.  This solution may be improved to put `PragmaExpr`  only  when the expression may be dependent on FP environment.

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

I see at least two issues with such solution:

1. There are cases when part of AST should be interpreted in other context than it was created. The example was presented earlier, this is constexpr function evaluation:

          #pragma STDC FENV_ACCESS ON
  	constexpr float func(float x, float y) { return x + y;	}
  	
  	#pragma STDC FENV_ROUND FE_DOWNWARD
  	float V1 = func(1.11, 2.22);
  	
  	#pragma STDC FENV_ROUND FE_UPWARD
  	float V2 = func(1.11, 2.22);

If FP environment is encoded in AST nodes, then the body of `func` should use `FE_DYNAMIC` as rounding mode. During evaluation such nodes this value should be replaced by the rounding mode taken from current context. Constant evaluator anyway should use global state and `RoundingMode::Dynamic` must be interpreted specifically.

2. This solution requires huge changes  to AST node implementation, as demonstrated by D76384 <https://reviews.llvm.org/D76384>. And what will we do with CallExpr and CastExpr, which already have TrailingObjects? How this solution can be extended? For instance, we may want to make ExprWithCleanups sensitive to FP environment so that it can restore previous state. We have to start with redesign of ExprWithCleanups so that it contains FPOptions. As this node already contains TrailingObjects it is not a trivial task. Any node that designates an operation that can depend on FP state should be reworked first.

In contrast, using special nodes to represent global state make this task transparently. No changes are required to existing AST nodes, only AST consumers that are aware of FP state need to be updated. This way looks more robust due to orthogonal and less invasive implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77545