Roman Lebedev via llvm-dev
2021-Aug-23 11:11 UTC
[llvm-dev] [cfe-dev] Intrinsic llvm::isnan
Thank you for posting the RFC! I do not believe we should conflate StrictFP support, and `-ffast-math` handling, these are two separate/separatable concerns. As for the latter, right now i'm not convinced that we should second-guess/override explicit user request. This is inconsistent, and does not match how at least the GCC deals with it. I think changing the status-quo (before said patch) should be a separate RFC, and that change should be undone until after that RFC is accepted. As for the latter, the main point of confusion is, why is `@llvm.isnan` still used in non-StrictFP code? The argument that we need `@llvm.isnan` because we *might* transition in and out of StrictFP section does not seem to hold for me, because https://llvm.org/docs/LangRef.html#constrainedfp says:> If any FP operation in a function is constrained then they all must be constrained. This is required for correct LLVM IR.So presumably when codegen'ing a function, we already know that we will use StrictFP ops, and that should be the knob to use `@llvm.isnan`, i think. Roman On Mon, Aug 23, 2021 at 1:57 PM Serge Pavlov via cfe-dev <cfe-dev at lists.llvm.org> wrote:> > Hi all, > > Some time ago a new intrinsic `llvm.isnan` was introduced, which is intended to represent IEEE-754 operation `isNaN` as well as a family of C library functions `isnan*`. Recently during post-commit review concern was raised (see https://reviews.llvm.org/D104854) that this functionality must have had RFC to make sure there is consensus on semantics. > > Previously the frontend intrinsic `__builtin_isnan` was converted into `cmp uno` during IR generation in clang codegen. There are two main reasons why this solution is not satisfactory. > > 1. Strict floating-point semantics. > > If FP exceptions are not ignored, `cmp uno` must be replaced with its constrained counterpart, namely `llvm.experimental.constrained.fcmp` or `llvm.experimental.constrained.fcmps`. None of them is compatible with the semantics of `isnan`. Both IEEE-754 (5.7.2) an C standard (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf, F.3p6) demand that this function does not raise floating point exceptions. Both the constrained compare intrinsics raise an exception if either operand is a SNAN (https://llvm.org/docs/LangRef.html#id1131). So there was no target-independent IR construct that could express `isnan`. > > This drawback was significant enough and some attempts to alleviate it were undertaken. In https://reviews.llvm.org/D95948 `isnan` was implemented using integer operations in strictfp functions. It however is not suitable for targets where a more efficient way exists, like dedicated instruction. Another solution was implemented in https://reviews.llvm.org/D96568, where a hook 'clang::TargetCodeGenInfo::testFPKind' was introduced, which injects target specific code into IR. Such a solution makes IR more target-dependent and prevents some IR-level optimizations. > > 2. Compilation with -ffast-math > > The option '-ffast-math' is often used for performance critical code, as it can produce faster code. In this case the user must ensure that NaNs are not used as operand values. `isnan` is just proposed for such checks, but it was unusable when `isnan` was represented by compare instruction, because the latter may be optimized out. One of use cases is data in memory, which is processed by a function compiled with `-ffast-math`. Some items in the data are NaNs to denote absence of values. > > This point requires some remarks about using NaNs when a function is compiled with `-ffast-math`. GCC manual does not specify how this option works, it only states about `-ffinite-math-only` (https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Optimize-Options.html#Optimize-Options): > > `Allow optimizations for floating-point arithmetic that assume that arguments and results are not NaNs or +-Infs.` > > `isnan` does not do any arithmetic, only check, so this statement apparently does not apply to it. There is a GCC bug report https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949, where investigation conforms that std::isnan() and std::fpclassify() should works with NaNs as specified even in -ffast-math mode. > > Extending NaN restrictions in -ffast-math mode to functions like `isnan` does not make code faster, but is a source of broken user expectations. If a user writes `isnan` they usually expect an actual check. Silently removing the check is a stronger action than assuming that float value contains only real numbers. > > Intrinsic `llvm.isnan` solves these problems. It > - represents the check throughout the IR pipeline and saves it from undesired optimizations, > - is lowered in selector, which can choose the most suitable implementation for particular target, > - helps keeping IR target-independent, > - facilitates program analysis as the operation is presented explicitly and is not hidden behind general nodes. > > Note that `llvm.isnan` is optimized out if its argument is an operation with `nnan` flag, this behavior agrees with the definition of this flag in LLVM documentation. > > Any feedback is welcome. > > Thanks, > --Serge > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Serge Pavlov via llvm-dev
2021-Aug-23 13:00 UTC
[llvm-dev] [cfe-dev] Intrinsic llvm::isnan
On Mon, Aug 23, 2021 at 6:12 PM Roman Lebedev <lebedev.ri at gmail.com> wrote:> Thank you for posting the RFC! > > I do not believe we should conflate StrictFP support, and > `-ffast-math` handling, these are two separate/separatable concerns. >You are right, they are separate, but they originate from the implementation of the same function and can be solved with the same solution.> > As for the latter, right now i'm not convinced that we should > second-guess/override explicit user request. > This is inconsistent, and does not match how at least the GCC deals with > it. > I think changing the status-quo (before said patch) should be a separate > RFC, > and that change should be undone until after that RFC is accepted. > >Actually we have two explicit user requests, a call of 'isnan' and an option '-ffast-math'. IMHO they do not contradict each other as 'isnan' is not an arithmetic operation. There is a discussion in https://reviews.llvm.org/D18513#387418, which also expresses the opinion that limitations imposed by '-ffast-math' should be applied only to 'math' functions but not to 'tests'. As for GCC behavior, they agree that this behavior is a bag: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949. Intel and Microsoft compilers do not replace 'isnan' with assumed value.> As for the latter, the main point of confusion is, > why is `@llvm.isnan` still used in non-StrictFP code? >We have to introduce an intrinsic to represent `isnan` in strictfp environment. It is natural to use it for the default environment as well. Besides, a target may have a more efficient way to represent `isnan` than unordered comparison. The argument that we need `@llvm.isnan` because we *might* transition> in and out of StrictFP section does not seem to hold for me, because > https://llvm.org/docs/LangRef.html#constrainedfp says: > > > If any FP operation in a function is constrained then they all must be > constrained. This is required for correct LLVM IR. >There was no such intention. The primary motivation was strict fp exceptions.> So presumably when codegen'ing a function, we already know that we > will use StrictFP ops, and that should be the knob to use `@llvm.isnan`, > i think. > > > Roman > > > > > On Mon, Aug 23, 2021 at 1:57 PM Serge Pavlov via cfe-dev > <cfe-dev at lists.llvm.org> wrote: > > > > Hi all, > > > > Some time ago a new intrinsic `llvm.isnan` was introduced, which is > intended to represent IEEE-754 operation `isNaN` as well as a family of C > library functions `isnan*`. Recently during post-commit review concern was > raised (see https://reviews.llvm.org/D104854) that this functionality > must have had RFC to make sure there is consensus on semantics. > > > > Previously the frontend intrinsic `__builtin_isnan` was converted into > `cmp uno` during IR generation in clang codegen. There are two main reasons > why this solution is not satisfactory. > > > > 1. Strict floating-point semantics. > > > > If FP exceptions are not ignored, `cmp uno` must be replaced with its > constrained counterpart, namely `llvm.experimental.constrained.fcmp` or > `llvm.experimental.constrained.fcmps`. None of them is compatible with the > semantics of `isnan`. Both IEEE-754 (5.7.2) an C standard ( > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf, F.3p6) demand > that this function does not raise floating point exceptions. Both the > constrained compare intrinsics raise an exception if either operand is a > SNAN (https://llvm.org/docs/LangRef.html#id1131). So there was no > target-independent IR construct that could express `isnan`. > > > > This drawback was significant enough and some attempts to alleviate it > were undertaken. In https://reviews.llvm.org/D95948 `isnan` was > implemented using integer operations in strictfp functions. It however is > not suitable for targets where a more efficient way exists, like dedicated > instruction. Another solution was implemented in > https://reviews.llvm.org/D96568, where a hook > 'clang::TargetCodeGenInfo::testFPKind' was introduced, which injects target > specific code into IR. Such a solution makes IR more target-dependent and > prevents some IR-level optimizations. > > > > 2. Compilation with -ffast-math > > > > The option '-ffast-math' is often used for performance critical code, as > it can produce faster code. In this case the user must ensure that NaNs are > not used as operand values. `isnan` is just proposed for such checks, but > it was unusable when `isnan` was represented by compare instruction, > because the latter may be optimized out. One of use cases is data in > memory, which is processed by a function compiled with `-ffast-math`. Some > items in the data are NaNs to denote absence of values. > > > > This point requires some remarks about using NaNs when a function is > compiled with `-ffast-math`. GCC manual does not specify how this option > works, it only states about `-ffinite-math-only` ( > https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Optimize-Options.html#Optimize-Options > ): > > > > `Allow optimizations for floating-point arithmetic that assume that > arguments and results are not NaNs or +-Infs.` > > > > `isnan` does not do any arithmetic, only check, so this statement > apparently does not apply to it. There is a GCC bug report > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949, where investigation > conforms that std::isnan() and std::fpclassify() should works with NaNs as > specified even in -ffast-math mode. > > > > Extending NaN restrictions in -ffast-math mode to functions like `isnan` > does not make code faster, but is a source of broken user expectations. If > a user writes `isnan` they usually expect an actual check. Silently > removing the check is a stronger action than assuming that float value > contains only real numbers. > > > > Intrinsic `llvm.isnan` solves these problems. It > > - represents the check throughout the IR pipeline and saves it from > undesired optimizations, > > - is lowered in selector, which can choose the most suitable > implementation for particular target, > > - helps keeping IR target-independent, > > - facilitates program analysis as the operation is presented explicitly > and is not hidden behind general nodes. > > > > Note that `llvm.isnan` is optimized out if its argument is an operation > with `nnan` flag, this behavior agrees with the definition of this flag in > LLVM documentation. > > > > Any feedback is welcome. > > > > Thanks, > > --Serge > > _______________________________________________ > > cfe-dev mailing list > > cfe-dev at lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210823/22d7cc8c/attachment-0001.html>