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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D77545#1977688 <https://reviews.llvm.org/D77545#1977688>, @sepavloff wrote:

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


As someone who's very familiar with how Clang and its various tools generally use the constant evaluator, I am telling you that reasoning by implication from the presence or absence of a direct `PragmaExpr` on an initializer is going to involve a lot of extremely subtle interactions and a very long tail of bugs.  Since those bugs are going to be localized to this one extremely obscure feature, I do not think that is a good idea.

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

(1) It is not a good idea to use actual global state to model machine state within a constant evaluation because information is extremely prone to leaking in and out.
(2) We are specifically *required* by the standard to ignore `FENV_ACCESS ON` pragmas in mandatory constant evaluation.

> 2. This solution requires huge changes  to AST node implementation, as demonstrated by D76384 <https://reviews.llvm.org/D76384>.

That patch has actually gotten much smaller.

> And what will we do with CallExpr and CastExpr, which already have TrailingObjects?

It's actually quite easy to add more trailing fields to an AST node that already has them.  Most of the boilerplate in D76384 <https://reviews.llvm.org/D76384> is from switching new-expessions to calls to static factories, but you don't need to do that when a node is already using factories.

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

This representation does not model these things as explicit FP environment state changes that would need to be undone on an exception.  But also, adding more trailing objects to a node is not a significant burden.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77545