[PATCH] D95948: Stop traping on sNaN in __builtin_isnan

[Melanie Blower via Phabricator via llvm-commits]

mibintc 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));
----------------
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.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3001
+    // NaN has all exp bits set and a non zero significand. Therefore:
+    // isnan(V) == ((abs(V) & exp mask) - exp mask < 0)
+    unsigned bitsize = Ty->getScalarSizeInBits();
----------------
Are you using a reference (e.g. existing implementation) for this rewrite, or is this invention? If a reference can you please tell me what it is.  The expression you have written here doesn't match the codegen below. I don't see the comparison to zero. Can you provide full parenthesization--the compare to zero is lower than subtract?


================
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));
----------------
compared to the comment above at line 3001, lhs and rhs are swapped in the sub


================
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)
----------------
the comment at line 3001 doesn't show the lshr or the compare to zero


================
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
----------------
Why did you put long double into this new test case instead of putting it with the float and double in strictfp_builtins?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95948