[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.h:622
     setFPContractMode(LangOptions::FPM_Off);
     setRoundingMode(static_cast<RoundingMode>(LangOptions::FPR_ToNearest));
     setFPExceptionMode(LangOptions::FPE_Ignore);
----------------
efriedma wrote:
> sepavloff wrote:
> > efriedma wrote:
> > > sepavloff wrote:
> > > > efriedma wrote:
> > > > > sepavloff wrote:
> > > > > > efriedma wrote:
> > > > > > > I'm suggesting this should be `setRoundingMode(llvm::RoundingMode::Dynamic)`.
> > > > > > > 
> > > > > > > If FENV_ACCESS is off, getEffectiveRoundingMode() converts that to "nearest".  If FENV_ACCESS is turned on, the mode is treated as dynamic.  This is exactly what we want.
> > > > > > It would change semantics. In particular, it would make impossible to use FP arithmetic in constexpr functions. An expression like `1.0 / 3.0` cannot be evaluated with dynamic rounding mode.
> > > > > Can we just change the relevant code in ExprConstant to call getEffectiveRoundingMode()?
> > > > What is the advantage of using FE_DYNAMIC in the case when it is known that rounding mode is FE_TONEAREST?
> > > > 
> > > > Probably `FENV_ROUND FE_DYNAMIC` should result in `dynamic` rounding even if `FENV_ACCESS ON` is absent. Rounding mode cannot be changed in this case but it can be used to inform the compiler that such function can be called in environment, where rounding mode is non-default. It would prevent some constant evaluations and other transformations that assume rounding mode is known. Anyway the standard only allow to assume FE_TONEAREST but does not require this.
> > > > 
> > > > In such case `getEffectiveRoundingMode` is not needed.
> > > I really want to keep the state we store, and the state updates, as simple as possible; if that means making getEffectiveRoundingMode() or whatever more complicated, that's fine.  It's a matter of making sure we understand all the relevant transitions.
> > > 
> > > As far as I can tell, according to the standard, the initial state for FENV_ROUND is supposed to be FE_DYNAMIC, as if "#pragma STDC FENV_ROUND FE_DYNAMIC" were written at the beginning of every file.  If we think we need a new state, something like FE_DYNAMIC_INITIAL, to represent the initial state, we can, I guess, but I don't think the standard text requires it.
> > > As far as I can tell, according to the standard, the initial state for FENV_ROUND is supposed to be FE_DYNAMIC, as if "#pragma STDC FENV_ROUND FE_DYNAMIC" were written at the beginning of every file. 
> > 
> > Could you please give any reference to this statement? I thought the initial state should be FE_TONEAREST, as it follows from F.8.3p1.
> " If no FENV_ROUND pragma is in effect, or the specified constant rounding mode is FE_DYNAMIC, rounding is according to the mode specified by the dynamic floating-point environment".  And all the other rules for FENV_ROUND only apply to "an FENV_ROUND pragma establishing a mode other than FE_DYNAMIC".  So no FENV_ROUND is equivalent to "FENV_ROUND FENV_DYNAMIC".
> 
> The text also says, "If the FE_DYNAMIC mode is specified and FENV_ACCESS is 'off', the translator may assume that the default rounding mode is in effect".  But that's the same result you'd get if you didn't specify FENV_ROUND at all: it's just combining the general rule that you're not allowed to mess with the dynamic rounding mode outside the scope of FENV_ACCESS, with the rule that the initial rounding mode is "nearest".
Thank you for the references. Indeed, default behavior specified by the standard is to use dynamic rounding. It however do not agree with the default compiler behavior - to use FE_TONEAREST. That's why we cannot make `setRoundingMode(llvm::RoundingMode::Dynamic)`. 

With options '-frounding-math' compiler conforms to the latest standard draft. In this case however FENV_ACCESS is not necessary. Using FE_DYNAMIC with subsequent deduction of actual rounding mode looks a fragile solution compared with setting actual mode in CurFPFeatures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126364