[PATCH] D88498: [FPEnv] Evaluate initializers in constant rounding mode

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

ping!



================
Comment at: clang/lib/Parse/ParseDecl.cpp:2290
+          // rounding mode.
+          if (VD->isFileVarDecl() || VD->isConstexpr() ||
+              (!getLangOpts().CPlusPlus && VD->isStaticLocal())) {
----------------
sepavloff wrote:
> sepavloff wrote:
> > rsmith wrote:
> > > It's far from clear to me that this is correct in C++. In principle, for a dynamic initializer, the rounding mode could have been set by an earlier initializer.
> > > 
> > > Perhaps we can make an argument that, due to the permission to interleave initializers from different TUs, every dynamic initializer must leave the program in the default rounding mode, but I don't think even that makes this approach correct, because an initializer could do this:
> > > 
> > > ```
> > > double d;
> > > double e = (fesetround(...), d = some calculation, fesetround(...default...), d);
> > > ```
> > > 
> > > I think we can only do this in C and will need something different for C++.
> > > 
> > > (I think this also has problems in C++ due to constexpr functions: it's not the case that all floating point operations that will be evaluated as part of the initializer lexically appear within it.)
> > I changed the code to confine it with C and constexpr only. Hopefully this would be enough to enable build of SPECS attempted by @mibintc (https://reviews.llvm.org/D87528#2295015). However in long-term perspective we should return to this code.
> > 
> > The intent was to align behavior of C and C++. If an initializer is valid in C, then it should produce the same code in C++. If the source code like
> > ```
> > float appx_coeffs_fp32[3 * 256] = {
> >     SEGMENT_BIAS + 1.4426950216,
> > …
> > ```
> > produces compact table in C mode and huge initialization code in C++, it would be strange from user viewpoint and would not give any advantage.
> > 
> > C in C2x presents pretty consistent model, provided that `#pragma STDC FENV_ROUND FE_DYNAMIC` does not set constant rounding mode. Initializers for variables with static allocation are always evaluated in constant rounding mode and user can chose the mode using pragma FENV_ROUND.
> > 
> > When extending this model to C++ we must solve the problem of dynamic initialization. It obviously occurs in runtime rounding mode, so changing between static and dynamic initialization may change semantics. If however dynamic initialization of global variable occurs in constant rounding mode (changing FP control modes in initializers without restoring them is UB), static and dynamic initialization would be semantically equivalent.
> > 
> > We cannot apply the same rule to local static variables, as they are treated differently in C and C++. So the code:
> > ```
> > float func_01(float x) {
> >   #pragma STDC FENV_ACCESS ON
> >   static float val = 1.0/3.0;
> >   return val;
> > }
> > ```
> > Would be compiled differently in C and C++.
> > 
> Additional note:
> 
> If initialization is dynamic and constant rounding mode is not default, the body of initializer is executed with dynamic rounding mode set to the constant mode. That is, the code:
> ```
>     #pragma STDC FENV_ROUND FE_UPWARD
>     float var = some_init_expr;
> ```
> generates code similar to:
> ```
>     float var = []()->float {
>       #pragma STDC FENV_ROUND FE_UPWARD
>       return some_init_expr;
>     }
> ```
> 
> So initializers of global variable must conform to:
> 
> 1. If the initialization is dynamic and dynamic rounding mode is changed in the initializer, it must be restored upon finishing the initializer.
> 2. The initializer is evaluated in constant rounding mode. If the initialization is dynamic, the initializing code is executed in dynamic rounding mode set to the constant rounding mode.
> 
> These seems enough to provide compatibility with C and the same semantics for static and dynamic initialization.
@rsmith Serge changed the patch to confine it with C and constexpr only, is that adequate to move forward with this patch, and we can return to C++ at some point down the road?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88498