[PATCH] D69798: Implement inlining of strictfp functions

Kevin P. Neal via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 20 12:11:55 PST 2019


kpn added a comment.

In D69798#1792906 <https://reviews.llvm.org/D69798#1792906>, @sepavloff wrote:

> In D69798#1792237 <https://reviews.llvm.org/D69798#1792237>, @craig.topper wrote:
>
> > In D69798#1792231 <https://reviews.llvm.org/D69798#1792231>, @sepavloff wrote:
> >
> > > When function that uses default FP environment is inlined into strictfp function, code of the former will be executed in the FP environment set in the strictfp function. To fix this behavior the FP environment should be saved upon entry to the inlined function, FP environment reset to default state, and the saved state must be restored upon leaving the inlined function.
> >
> >
> > So if the function is inlined it would use the reset state, but if it doesn't get inlined it would use the caller's state. Doesn't that mean whether or not the compiler inlines the function changes the behavior of the program?
>
>
> It is so now. If a code in which `#pargma STDC FENV_ACCESS ON` acts calls an external function, it would be executed in caller's FP environment. This is wrong if the callee expects default one. We could put `get_fenv`, `reset_fenv` and `set_fenv`, introduced in D71742 <https://reviews.llvm.org/D71742>, around all calls, unless the called function has `strictfp` attribute, or we know that it does not use FP operation. It however could create unneeded code, which is bad for performance. We need to elaborate proper solution.


If FENV_ACCESS is ON and a function is called that expects it to be OFF then isn't that just plain undefined behavior? Unless it was changed since C99 I don't see how this is the compiler's problem to solve. And the compiler really shouldn't be changing the FP environment implicitly just because an arbitrary function was called, or we fell out of an FENV_ACCESS=ON scope, or whatever.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69798





More information about the llvm-commits mailing list