[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:2995
+        Builder.getDefaultConstrainedExcept() == fp::ebIgnore ||
+        !Ty->isIEEE()) {
+      V = Builder.CreateFCmpUNO(V, V, "cmp");
----------------
I'm not too sure if that's enough to check isIEEE here. x86 extended precision behaves a bit different to IEEE single and double precision, esp. wrt. infinite and NaN. I've decided to ignore NaN values that are deprecated since the 8087.


================
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();
----------------
mibintc wrote:
> 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?
I've noticed glibc isnan implementation does not trigger a trap so I've looked at the assembly it generates for the float case. This is the generalized version for double and long double as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95948