[PATCH] D95948: Stop traping on sNaN in __builtin_isnan

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked 2 inline comments as done.
thopre added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2988
     CodeGenFunction::CGFPOptionsRAII FPOptsRAII(*this, E);
-    // FIXME: for strictfp/IEEE-754 we need to not trap on SNaN here.
     Value *V = EmitScalarExpr(E->getArg(0));
----------------
mibintc wrote:
> What about all the other FIXME related to strictfp, are we going to pick them off piecemeal? It would be nice to have a holistic approach, it would be more clear, and less duplicated code. For example, presumably the test at line 2991 is common.
I do want to do isinf and other similar builtin but I would prefer to have a review of this to see if there's anything wrong with my approach before I work on other builtins. Once I do I'll factor things out as needed.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3010
+    Value *Sub =
+        Builder.CreateSub(llvm::ConstantInt::get(IntTy, ExpMask), AbsV);
+    V = Builder.CreateLShr(Sub, llvm::ConstantInt::get(IntTy, bitsize - 1));
----------------
mibintc wrote:
> compared to the comment above at line 3001, lhs and rhs are swapped in the sub
My bad, the comment needs to be fixed.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3011
+        Builder.CreateSub(llvm::ConstantInt::get(IntTy, ExpMask), AbsV);
+    V = Builder.CreateLShr(Sub, llvm::ConstantInt::get(IntTy, bitsize - 1));
+    if (bitsize > 32)
----------------
mibintc wrote:
> the comment at line 3001 doesn't show the lshr or the compare to zero
This logical right shift will move the sign bit to bit0 and set all other bits to 0. The result will be 1 (true) if sign bit is set (sub is negative) or 0 (false) otherwise. That matches the comment. Would you like me to add a comment here to make it more explicit?


================
Comment at: clang/test/CodeGen/X86/strictfp_builtins.c:1
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 %s -emit-llvm -ffp-exception-behavior=maytrap -o - -triple x86_64-unknown-unknown | FileCheck %s
----------------
mibintc wrote:
> Why did you put long double into this new test case instead of putting it with the float and double in strictfp_builtins?
long double is implemented differently for each target. x86 uses an 80 bit extended precision format, AFAIK AArch64 uses a 128bit IEEE format, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95948