Stop traping on sNaN in __builtin_isnan

[thopre]

Summary

__builtin_isnan currently generates a floating-point compare operation
which triggers a trap when faced with a signaling NaN in StrictFP mode.
This commit uses integer operations instead to not generate any trap in
such a case.

[mibintc]

clang/lib/CodeGen/CGBuiltin.cpp

2988

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.

3001

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?

3010

compared to the comment above at line 3001, lhs and rhs are swapped in the sub

3011

the comment at line 3001 doesn't show the lshr or the compare to zero

clang/test/CodeGen/X86/strictfp_builtins.c

2

Why did you put long double into this new test case instead of putting it with the float and double in strictfp_builtins?

[thopre]

clang/lib/CodeGen/CGBuiltin.cpp

2988

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.

3010

My bad, the comment needs to be fixed.

3011

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?

clang/test/CodeGen/X86/strictfp_builtins.c

2

long double is implemented differently for each target. x86 uses an 80 bit extended precision format, AFAIK AArch64 uses a 128bit IEEE format, etc.

[thopre]

clang/lib/CodeGen/CGBuiltin.cpp

2995

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.

3001

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.

[thopre]

• Fix order of sub operands in comments and use fully parenthesized expression
• Explain how LShr is equivalent to < 0

[mibintc]

clang/lib/CodeGen/CGBuiltin.cpp

3011

No, that's OK, you don't need to do that. thanks. I'd like to see what other reviewers say, thanks for working on this!

[kpn]

This looks like a definite step forward. Thank you!

Can you do an AArch64 test case showing long double? Right now there's no 128-bit test case.

[mibintc]

Should we add tests for mlong-double-64, -80, -128?

[kpn]

In D95948#2542884, @mibintc wrote:

Should we add tests for mlong-double-64, -80, -128?

Assuming Ty->getScalarSizeInBits() returns 64, 80, or 128 in those cases, I think it's good enough to just add an AArch64 (or some other 128-bit long double host) and call it a day. We're testing isnan(), not that command line option, after all.

[thopre]

Add AArch64 testcase and rebase

clang/test/CodeGen/aarch64-strictfp-builtins.c

1

AArch64 is not StrictFP enabled so we need this option.

[kpn]

LGTM

clang/lib/CodeGen/CGBuiltin.cpp

 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/OSLog.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "llvm/ADT/APFloat.h"
+#include "llvm/ADT/APInt.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/IntrinsicsAArch64.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
case Builtin::BI__builtin_isunordered:
       LHS = Builder.CreateFCmpUNO(LHS, RHS, "cmp");
       break;
     }
     // ZExt bool to int type.
     return RValue::get(Builder.CreateZExt(LHS, ConvertType(E->getType())));
   }
   case Builtin::BI__builtin_isnan: {
     CodeGenFunction::CGFPOptionsRAII FPOptsRAII(*this, E);
-    // FIXME: for strictfp/IEEE-754 we need to not trap on SNaN here.
     Value *V = EmitScalarExpr(E->getArg(0));
+    llvm::Type *Ty = V->getType();
+    const llvm::fltSemantics &Semantics = Ty->getFltSemantics();
+    if (!Builder.getIsFPConstrained() ||
+        Builder.getDefaultConstrainedExcept() == fp::ebIgnore ||
+        !Ty->isIEEE()) {
       V = Builder.CreateFCmpUNO(V, V, "cmp");
       return RValue::get(Builder.CreateZExt(V, ConvertType(E->getType())));
     }
 
+    // NaN has all exp bits set and a non zero significand. Therefore:
+    // isnan(V) == ((exp mask - (abs(V) & exp mask)) < 0)
+    unsigned bitsize = Ty->getScalarSizeInBits();
+    llvm::IntegerType *IntTy = Builder.getIntNTy(bitsize);
+    Value *IntV = Builder.CreateBitCast(V, IntTy);
+    APInt AndMask = APInt::getSignedMaxValue(bitsize);
+    Value *AbsV =
+        Builder.CreateAnd(IntV, llvm::ConstantInt::get(IntTy, AndMask));
+    APInt ExpMask = APFloat::getInf(Semantics).bitcastToAPInt();
+    Value *Sub =
+        Builder.CreateSub(llvm::ConstantInt::get(IntTy, ExpMask), AbsV);
+    // V = sign bit (Sub) <=> V = (Sub < 0)
+    V = Builder.CreateLShr(Sub, llvm::ConstantInt::get(IntTy, bitsize - 1));
+    if (bitsize > 32)
+      V = Builder.CreateTrunc(V, ConvertType(E->getType()));
+    return RValue::get(V);
+  }
+
   case Builtin::BI__builtin_matrix_transpose: {
     const auto *MatrixTy = E->getArg(0)->getType()->getAs<ConstantMatrixType>();
     Value *MatValue = EmitScalarExpr(E->getArg(0));
     MatrixBuilder<CGBuilderTy> MB(Builder);
     Value *Result = MB.CreateMatrixTranspose(MatValue, MatrixTy->getNumRows(),
                                              MatrixTy->getNumColumns());
     return RValue::get(Result);
   }

clang/test/CodeGen/X86/strictfp_builtins.c

+// 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
+
+// Test that the constrained intrinsics are picking up the exception
+// metadata from the AST instead of the global default from the command line.
+// FIXME: these functions shouldn't trap on SNaN.
+
+#pragma float_control(except, on)
+
+int printf(const char *, ...);
+
+// CHECK-LABEL: @p(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[STR_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:    [[X_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i8* [[STR:%.*]], i8** [[STR_ADDR]], align 8
+// CHECK-NEXT:    store i32 [[X:%.*]], i32* [[X_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load i8*, i8** [[STR_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, i32* [[X_ADDR]], align 4
+// CHECK-NEXT:    [[CALL:%.*]] = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str, i64 0, i64 0), i8* [[TMP0]], i32 [[TMP1]]) [[ATTR4:#.*]]
+// CHECK-NEXT:    ret void
+//
+void p(char *str, int x) {
+  printf("%s: %d\n", str, x);
+}
+
+#define P(n,args) p(#n #args, __builtin_##n args)
+
+// CHECK-LABEL: @test_long_double_isnan(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[LD_ADDR:%.*]] = alloca x86_fp80, align 16
+// CHECK-NEXT:    store x86_fp80 [[D:%.*]], x86_fp80* [[LD_ADDR]], align 16
+// CHECK-NEXT:    [[TMP0:%.*]] = load x86_fp80, x86_fp80* [[LD_ADDR]], align 16
+// CHECK-NEXT:    [[BITCAST:%.*]] = bitcast x86_fp80 [[TMP0]] to i80
+// CHECK-NEXT:    [[ABS:%.*]] = and i80 [[BITCAST]], 604462909807314587353087
+// CHECK-NEXT:    [[TMP1:%.*]] = sub i80 604453686435277732577280, [[ABS]]
+// CHECK-NEXT:    [[ISNAN:%.*]] = lshr i80 [[TMP1]], 79
+// CHECK-NEXT:    [[RES:%.*]] = trunc i80 [[ISNAN]] to i32
+// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @.str.1, i64 0, i64 0), i32 [[RES]]) [[ATTR4]]
+// CHECK-NEXT:    ret void
+//
+void test_long_double_isnan(long double ld) {
+  P(isnan, (ld));
+
+  return;
+}

clang/test/CodeGen/aarch64-strictfp-builtins.c

+// RUN: %clang_cc1 %s -emit-llvm -ffp-exception-behavior=maytrap -fexperimental-strict-floating-point -o - -triple arm64-none-linux-gnu | FileCheck %s
+
+// Test that the constrained intrinsics are picking up the exception
+// metadata from the AST instead of the global default from the command line.
+
+#pragma float_control(except, on)
+
+int printf(const char *, ...);
+
+// CHECK-LABEL: @p(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[STR_ADDR:%.*]] = alloca i8*, align 8
+// CHECK-NEXT:    [[X_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i8* [[STR:%.*]], i8** [[STR_ADDR]], align 8
+// CHECK-NEXT:    store i32 [[X:%.*]], i32* [[X_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load i8*, i8** [[STR_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, i32* [[X_ADDR]], align 4
+// CHECK-NEXT:    [[CALL:%.*]] = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str, i64 0, i64 0), i8* [[TMP0]], i32 [[TMP1]])
+// CHECK-NEXT:    ret void
+//
+void p(char *str, int x) {
+  printf("%s: %d\n", str, x);
+}
+
+#define P(n,args) p(#n #args, __builtin_##n args)
+
+// CHECK-LABEL: @test_long_double_isnan(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[LD_ADDR:%.*]] = alloca fp128, align 16
+// CHECK-NEXT:    store fp128 [[D:%.*]], fp128* [[LD_ADDR]], align 16
+// CHECK-NEXT:    [[TMP0:%.*]] = load fp128, fp128* [[LD_ADDR]], align 16
+// CHECK-NEXT:    [[BITCAST:%.*]] = bitcast fp128 [[TMP0]] to i128
+// CHECK-NEXT:    [[ABS:%.*]] = and i128 [[BITCAST]], 170141183460469231731687303715884105727
+// CHECK-NEXT:    [[TMP1:%.*]] = sub i128 170135991163610696904058773219554885632, [[ABS]]
+// CHECK-NEXT:    [[ISNAN:%.*]] = lshr i128 [[TMP1]], 127
+// CHECK-NEXT:    [[RES:%.*]] = trunc i128 [[ISNAN]] to i32
+// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @.str.1, i64 0, i64 0), i32 [[RES]])
+// CHECK-NEXT:    ret void
+//
+void test_long_double_isnan(long double ld) {
+  P(isnan, (ld));
+
+  return;
+}

clang/test/CodeGen/strictfp_builtins.c

 // CHECK-NEXT:    ret void
 //
 void test_isinf_sign(double d) {
   P(isinf_sign, (d));
 
   return;
 }
 
-// CHECK-LABEL: @test_isnan(
+// CHECK-LABEL: @test_float_isnan(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[F_ADDR:%.*]] = alloca float, align 4
+// CHECK-NEXT:    store float [[F:%.*]], float* [[F_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load float, float* [[F_ADDR]], align 4
+// CHECK-NEXT:    [[BITCAST:%.*]] = bitcast float [[TMP0]] to i32
+// CHECK-NEXT:    [[ABS:%.*]] = and i32 [[BITCAST]], [[#%u,0x7FFFFFFF]]
+// CHECK-NEXT:    [[TMP1:%.*]] = sub i32 [[#%u,0x7F800000]], [[ABS]]
+// CHECK-NEXT:    [[ISNAN:%.*]] = lshr i32 [[TMP1]], 31
+// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @.str.4, i64 0, i64 0), i32 [[ISNAN]]) [[ATTR4]]
+// CHECK-NEXT:    ret void
+//
+void test_float_isnan(float f) {
+  P(isnan, (f));
+
+  return;
+}
+
+// CHECK-LABEL: @test_double_isnan(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    [[D_ADDR:%.*]] = alloca double, align 8
 // CHECK-NEXT:    store double [[D:%.*]], double* [[D_ADDR]], align 8
 // CHECK-NEXT:    [[TMP0:%.*]] = load double, double* [[D_ADDR]], align 8
-// CHECK-NEXT:    [[CMP:%.*]] = call i1 @llvm.experimental.constrained.fcmp.f64(double [[TMP0]], double [[TMP0]], metadata !"uno", metadata !"fpexcept.strict") [[ATTR4]]
-// CHECK-NEXT:    [[TMP1:%.*]] = zext i1 [[CMP]] to i32
-// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @.str.4, i64 0, i64 0), i32 [[TMP1]]) [[ATTR4]]
+// CHECK-NEXT:    [[BITCAST:%.*]] = bitcast double [[TMP0]] to i64
+// CHECK-NEXT:    [[ABS:%.*]] = and i64 [[BITCAST]], [[#%u,0x7FFFFFFFFFFFFFFF]]
+// CHECK-NEXT:    [[TMP1:%.*]] = sub i64 [[#%u,0x7FF0000000000000]], [[ABS]]
+// CHECK-NEXT:    [[ISNAN:%.*]] = lshr i64 [[TMP1]], 63
+// CHECK-NEXT:    [[RES:%.*]] = trunc i64 [[ISNAN]] to i32
+// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @.str.5, i64 0, i64 0), i32 [[RES]]) [[ATTR4]]
 // CHECK-NEXT:    ret void
 //
-void test_isnan(double d) {
+void test_double_isnan(double d) {
   P(isnan, (d));
 
   return;
 }
 
 // CHECK-LABEL: @test_isnormal(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    [[D_ADDR:%.*]] = alloca double, align 8
 // CHECK-NEXT:    store double [[D:%.*]], double* [[D_ADDR]], align 8
 // CHECK-NEXT:    [[TMP0:%.*]] = load double, double* [[D_ADDR]], align 8
 // CHECK-NEXT:    [[ISEQ:%.*]] = call i1 @llvm.experimental.constrained.fcmp.f64(double [[TMP0]], double [[TMP0]], metadata !"oeq", metadata !"fpexcept.strict") [[ATTR4]]
 // CHECK-NEXT:    [[TMP1:%.*]] = call double @llvm.fabs.f64(double [[TMP0]]) [[ATTR5]]
 // CHECK-NEXT:    [[ISINF:%.*]] = call i1 @llvm.experimental.constrained.fcmp.f64(double [[TMP1]], double 0x7FF0000000000000, metadata !"ult", metadata !"fpexcept.strict") [[ATTR4]]
 // CHECK-NEXT:    [[ISNORMAL:%.*]] = call i1 @llvm.experimental.constrained.fcmp.f64(double [[TMP1]], double 0x10000000000000, metadata !"uge", metadata !"fpexcept.strict") [[ATTR4]]
 // CHECK-NEXT:    [[AND:%.*]] = and i1 [[ISEQ]], [[ISINF]]
 // CHECK-NEXT:    [[AND1:%.*]] = and i1 [[AND]], [[ISNORMAL]]
 // CHECK-NEXT:    [[TMP2:%.*]] = zext i1 [[AND1]] to i32
-// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([12 x i8], [12 x i8]* @.str.5, i64 0, i64 0), i32 [[TMP2]]) [[ATTR4]]
+// CHECK-NEXT:    call void @p(i8* getelementptr inbounds ([12 x i8], [12 x i8]* @.str.6, i64 0, i64 0), i32 [[TMP2]]) [[ATTR4]]
 // CHECK-NEXT:    ret void
 //
 void test_isnormal(double d) {
   P(isnormal, (d));
 
   return;
 }

llvm/include/llvm/ADT/APFloat.h

public:
   const fltSemantics &getSemantics() const { return *U.semantics; }
   bool isNonZero() const { return !isZero(); }
   bool isFiniteNonZero() const { return isFinite() && !isZero(); }
   bool isPosZero() const { return isZero() && !isNegative(); }
   bool isNegZero() const { return isZero() && isNegative(); }
   bool isSmallest() const { APFLOAT_DISPATCH_ON_SEMANTICS(isSmallest()); }
   bool isLargest() const { APFLOAT_DISPATCH_ON_SEMANTICS(isLargest()); }
   bool isInteger() const { APFLOAT_DISPATCH_ON_SEMANTICS(isInteger()); }
+  bool isIEEE() const { return usesLayout<IEEEFloat>(getSemantics()); }
 
   APFloat &operator=(const APFloat &RHS) = default;
   APFloat &operator=(APFloat &&RHS) = default;
 
   void toString(SmallVectorImpl<char> &Str, unsigned FormatPrecision = 0,
                 unsigned FormatMaxPadding = 3, bool TruncateZero = true) const {
     APFLOAT_DISPATCH_ON_SEMANTICS(
         toString(Str, FormatPrecision, FormatMaxPadding, TruncateZero));

llvm/include/llvm/IR/Type.h

public:
   /// type.
   unsigned getScalarSizeInBits() const LLVM_READONLY;
 
   /// Return the width of the mantissa of this type. This is only valid on
   /// floating-point types. If the FP type does not have a stable mantissa (e.g.
   /// ppc long double), this method returns -1.
   int getFPMantissaWidth() const;
 
+  /// Return whether the type is IEEE compatible, as defined by the eponymous
+  /// method in APFloat.
+  bool isIEEE() const { return APFloat::getZero(getFltSemantics()).isIEEE(); }
+
   /// If this is a vector type, return the element type, otherwise return
   /// 'this'.
   inline Type *getScalarType() const {
     if (isVectorTy())
       return getContainedType(0);
     return const_cast<Type *>(this);
   }