[CFE, SystemZ] Emit s390.tdc instrincic for __builtin_isnan in Constrained FP mode.

[jonpa]

Summary

The recent commit 00a6254 "Stop traping on sNaN in builtin_isnan" changed the lowering in constrained FP mode of builtin_isnan from an FP comparison to integer operations to avoid trapping.

SystemZ has a special instruction "Test Data Class" which is the preferred way to do this check. This patch adds a new target hook "emit_FPConstrained_builtin_isnan()" that lets SystemZ emit the s390_tdc intrinsic instead.

[thopre]

That's interesting. I presume that can be used to implement isinf as well? Perhaps better call the hook fpclassify or similar?

[uweigand]

In D96568#2559336, @thopre wrote:

That's interesting. I presume that can be used to implement isinf as well? Perhaps better call the hook fpclassify or similar?

Hmm, the instruction doesn't really implement fpclassify in itself, it is more like a combined check for fpclassify() == <some constant>. Specifically, the TEST DATA CLASS instruction takes an immediate operand that represents a bit mask, which each bit standing for one type of floating-point value (zero, normal, subnormal, infinity, QNaN, SNaN -- each in positive and negative versions). The instruction sets the condition code depending on whether the input FP number is in one of the classes selected by the bit mask, or not.

This is why Jonas' patch uses a bit mask of 0x0F -- this has the bits for the four types of NaN set (pos/neg QNaN/SNan). The instruction could indeed also be used to implement an isinf check (bit mask 0x30) or many other checks. We actually have a SystemZ back-end pass that tries to multiple combine FP checks into a single TEST DATA CLASS instruction.

However, the instruction does not directly implement the fpclassify semantics. To implement fpclassify, you'd still have to use multiple invocations of the instruction with different flags to determine the fpclassify output value.

[thopre]

In D96568#2559475, @uweigand wrote:

In D96568#2559336, @thopre wrote:

That's interesting. I presume that can be used to implement isinf as well? Perhaps better call the hook fpclassify or similar?

Hmm, the instruction doesn't really implement fpclassify in itself, it is more like a combined check for fpclassify() == <some constant>. Specifically, the TEST DATA CLASS instruction takes an immediate operand that represents a bit mask, which each bit standing for one type of floating-point value (zero, normal, subnormal, infinity, QNaN, SNaN -- each in positive and negative versions). The instruction sets the condition code depending on whether the input FP number is in one of the classes selected by the bit mask, or not.

This is why Jonas' patch uses a bit mask of 0x0F -- this has the bits for the four types of NaN set (pos/neg QNaN/SNan). The instruction could indeed also be used to implement an isinf check (bit mask 0x30) or many other checks. We actually have a SystemZ back-end pass that tries to multiple combine FP checks into a single TEST DATA CLASS instruction.

However, the instruction does not directly implement the fpclassify semantics. To implement fpclassify, you'd still have to use multiple invocations of the instruction with different flags to determine the fpclassify output value.

I see. I'm not sure whether it's better to have several target hooks or a single one like testFPKind that would take a flag saying what do we want to test (NaN, Inf, etc.).

[jonpa]

In D96568#2559476, @thopre wrote:

In D96568#2559475, @uweigand wrote:

In D96568#2559336, @thopre wrote:

That's interesting. I presume that can be used to implement isinf as well? Perhaps better call the hook fpclassify or similar?

Hmm, the instruction doesn't really implement fpclassify in itself, it is more like a combined check for fpclassify() == <some constant>. Specifically, the TEST DATA CLASS instruction takes an immediate operand that represents a bit mask, which each bit standing for one type of floating-point value (zero, normal, subnormal, infinity, QNaN, SNaN -- each in positive and negative versions). The instruction sets the condition code depending on whether the input FP number is in one of the classes selected by the bit mask, or not.

This is why Jonas' patch uses a bit mask of 0x0F -- this has the bits for the four types of NaN set (pos/neg QNaN/SNan). The instruction could indeed also be used to implement an isinf check (bit mask 0x30) or many other checks. We actually have a SystemZ back-end pass that tries to multiple combine FP checks into a single TEST DATA CLASS instruction.

However, the instruction does not directly implement the fpclassify semantics. To implement fpclassify, you'd still have to use multiple invocations of the instruction with different flags to determine the fpclassify output value.

I see. I'm not sure whether it's better to have several target hooks or a single one like testFPKind that would take a flag saying what do we want to test (NaN, Inf, etc.).

Personally I think testFPKind should work well - it gives a single target hook for related purposes which should be more readable, and it is also natural for SystemZ since we will be using the same (Test Data Class) instruction for differnet checks only with different flag bits... Any one else has an opinion? Should I go ahead and change the patch to this end?

[thopre]

In D96568#2567145, @jonpa wrote:

In D96568#2559476, @thopre wrote:

In D96568#2559475, @uweigand wrote:

In D96568#2559336, @thopre wrote:

That's interesting. I presume that can be used to implement isinf as well? Perhaps better call the hook fpclassify or similar?

Hmm, the instruction doesn't really implement fpclassify in itself, it is more like a combined check for fpclassify() == <some constant>. Specifically, the TEST DATA CLASS instruction takes an immediate operand that represents a bit mask, which each bit standing for one type of floating-point value (zero, normal, subnormal, infinity, QNaN, SNaN -- each in positive and negative versions). The instruction sets the condition code depending on whether the input FP number is in one of the classes selected by the bit mask, or not.

This is why Jonas' patch uses a bit mask of 0x0F -- this has the bits for the four types of NaN set (pos/neg QNaN/SNan). The instruction could indeed also be used to implement an isinf check (bit mask 0x30) or many other checks. We actually have a SystemZ back-end pass that tries to multiple combine FP checks into a single TEST DATA CLASS instruction.

However, the instruction does not directly implement the fpclassify semantics. To implement fpclassify, you'd still have to use multiple invocations of the instruction with different flags to determine the fpclassify output value.

I see. I'm not sure whether it's better to have several target hooks or a single one like testFPKind that would take a flag saying what do we want to test (NaN, Inf, etc.).

Personally I think testFPKind should work well - it gives a single target hook for related purposes which should be more readable, and it is also natural for SystemZ since we will be using the same (Test Data Class) instruction for differnet checks only with different flag bits... Any one else has an opinion? Should I go ahead and change the patch to this end?

Sounds good to me. Hopefully I'll get round to __builtin_isinf soon and a single hook will make the patch slightly smaller.

[jonpa]

Sounds good to me. Hopefully I'll get round to __builtin_isinf soon and a single hook will make the patch slightly smaller.

Patch updated to call the new hook testFPKind() and make it take a BuiltinID as argument (that seems to work at least for the moment - maybe an enum type will become necessary at some point per your suggestion..?)

I am not sure if this is "only" or "typically" used in constrained FP mode, or if the mode should be independent of calling this hook. The patch as it is asserts that it is called for an FP type but leaves it to the target to decide based on the FP mode, where SystemZ opts out unless it is constrained (which I think is what is wanted...).

[thopre]

In D96568#2569296, @jonpa wrote:

Sounds good to me. Hopefully I'll get round to __builtin_isinf soon and a single hook will make the patch slightly smaller.

Patch updated to call the new hook testFPKind() and make it take a BuiltinID as argument (that seems to work at least for the moment - maybe an enum type will become necessary at some point per your suggestion..?)

I am not sure if this is "only" or "typically" used in constrained FP mode, or if the mode should be independent of calling this hook. The patch as it is asserts that it is called for an FP type but leaves it to the target to decide based on the FP mode, where SystemZ opts out unless it is constrained (which I think is what is wanted...).

LGTM, we can adapt the hook later if needed. I do not know whether allowing the hook to be used for non constrained FP will prove useful but it is easy enough to ignore it for non FP so why not. Thanks for changing that!

[jonpa]

In D96568#2569710, @thopre wrote:

In D96568#2569296, @jonpa wrote:

Sounds good to me. Hopefully I'll get round to __builtin_isinf soon and a single hook will make the patch slightly smaller.

Patch updated to call the new hook testFPKind() and make it take a BuiltinID as argument (that seems to work at least for the moment - maybe an enum type will become necessary at some point per your suggestion..?)

I am not sure if this is "only" or "typically" used in constrained FP mode, or if the mode should be independent of calling this hook. The patch as it is asserts that it is called for an FP type but leaves it to the target to decide based on the FP mode, where SystemZ opts out unless it is constrained (which I think is what is wanted...).

LGTM, we can adapt the hook later if needed. I do not know whether allowing the hook to be used for non constrained FP will prove useful but it is easy enough to ignore it for non FP so why not. Thanks for changing that!

Thanks for review!

@uweigand: looks good on the SystemZ parts?

[uweigand]

LGTM as well, thanks!

[thopre]

In D96568#2569296, @jonpa wrote:

Sounds good to me. Hopefully I'll get round to __builtin_isinf soon and a single hook will make the patch slightly smaller.

Patch updated to call the new hook testFPKind() and make it take a BuiltinID as argument (that seems to work at least for the moment - maybe an enum type will become necessary at some point per your suggestion..?)

I am not sure if this is "only" or "typically" used in constrained FP mode, or if the mode should be independent of calling this hook. The patch as it is asserts that it is called for an FP type but leaves it to the target to decide based on the FP mode, where SystemZ opts out unless it is constrained (which I think is what is wanted...).

I forgot to ask at the time, why do you restrict this code to the constrained case? Presumably it's a lot faster than fabs+cmp and should be faster in all cases?

[jonpa]

In D96568#2832781, @thopre wrote:

In D96568#2569296, @jonpa wrote:

Sounds good to me. Hopefully I'll get round to __builtin_isinf soon and a single hook will make the patch slightly smaller.

Patch updated to call the new hook testFPKind() and make it take a BuiltinID as argument (that seems to work at least for the moment - maybe an enum type will become necessary at some point per your suggestion..?)

I am not sure if this is "only" or "typically" used in constrained FP mode, or if the mode should be independent of calling this hook. The patch as it is asserts that it is called for an FP type but leaves it to the target to decide based on the FP mode, where SystemZ opts out unless it is constrained (which I think is what is wanted...).

I forgot to ask at the time, why do you restrict this code to the constrained case? Presumably it's a lot faster than fabs+cmp and should be faster in all cases?

There are later optimizations that does this already in place (SystemZTDCPass). I checked now and it seems to make no difference at all to do this in the front-end always in non-constrained FP mode... (SPEC)

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_isnan: {
     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())));
     }
 
+    if (Value *Result = getTargetHooks().testFPKind(V, BuiltinID, Builder, CGM))
+      return RValue::get(Result);
+
     // 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));

clang/lib/CodeGen/TargetInfo.h

 //===---- TargetInfo.h - Encapsulate target details -------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // These classes wrap the information about a call or function
 // definition used to handle ABI compliancy.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_CODEGEN_TARGETINFO_H
 #define LLVM_CLANG_LIB_CODEGEN_TARGETINFO_H
 
+#include "CGBuilder.h"
 #include "CodeGenModule.h"
 #include "CGValue.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SyncScope.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 
public:
   /// by the system in a return slot.
   ///
   /// Used by __builtin_frob_return_addr().
   virtual llvm::Value *encodeReturnAddress(CodeGen::CodeGenFunction &CGF,
                                            llvm::Value *Address) const {
     return Address;
   }
 
+  /// Performs a target specific test of a floating point value for things
+  /// like IsNaN, Infinity, ... Nullptr is returned if no implementation
+  /// exists.
+  virtual llvm::Value *
+  testFPKind(llvm::Value *V, unsigned BuiltinID, CGBuilderTy &Builder,
+             CodeGenModule &CGM) const {
+    assert(V->getType()->isFloatingPointTy() && "V should have an FP type.");
+    return nullptr;
+  }
+
   /// Corrects the low-level LLVM type for a given constraint and "usual"
   /// type.
   ///
   /// \returns A pointer to a new LLVM type, possibly the same as the original
   /// on success; 0 on failure.
   virtual llvm::Type *adjustInlineAsmType(CodeGen::CodeGenFunction &CGF,
                                           StringRef Constraint,
                                           llvm::Type *Ty) const {

clang/lib/CodeGen/TargetInfo.cpp

 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGValue.h"
 #include "CodeGenFunction.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/IntrinsicsNVPTX.h"
+#include "llvm/IR/IntrinsicsS390.h"
 #include "llvm/IR/Type.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm> // std::sort
 
 using namespace clang;
 using namespace CodeGen;
 
 // Helper for coercing an aggregate argument or return value into an integer
public:
   }
 };
 
 class SystemZTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   SystemZTargetCodeGenInfo(CodeGenTypes &CGT, bool HasVector, bool SoftFloatABI)
       : TargetCodeGenInfo(
             std::make_unique<SystemZABIInfo>(CGT, HasVector, SoftFloatABI)) {}
-};
 
+  llvm::Value *testFPKind(llvm::Value *V, unsigned BuiltinID,
+                          CGBuilderTy &Builder,
+                          CodeGenModule &CGM) const override {
+    assert(V->getType()->isFloatingPointTy() && "V should have an FP type.");
+    // Only use TDC in constrained FP mode.
+    if (!Builder.getIsFPConstrained())
+      return nullptr;
+
+    llvm::Type *Ty = V->getType();
+    if (Ty->isFloatTy() || Ty->isDoubleTy() || Ty->isFP128Ty()) {
+      llvm::Module &M = CGM.getModule();
+      auto &Ctx = M.getContext();
+      llvm::Function *TDCFunc =
+          llvm::Intrinsic::getDeclaration(&M, llvm::Intrinsic::s390_tdc, Ty);
+      unsigned TDCBits = 0;
+      switch (BuiltinID) {
+      case Builtin::BI__builtin_isnan:
+        TDCBits = 0xf;
+        break;
+      default:
+        break;
+      }
+      if (TDCBits)
+        return Builder.CreateCall(
+            TDCFunc,
+            {V, llvm::ConstantInt::get(llvm::Type::getInt64Ty(Ctx), TDCBits)});
+    }
+    return nullptr;
+  }
+};
 }
 
 bool SystemZABIInfo::isPromotableIntegerTypeForABI(QualType Ty) const {
   // Treat an enum type as its underlying type.
   if (const EnumType *EnumTy = Ty->getAs<EnumType>())
     Ty = EnumTy->getDecl()->getIntegerType();
 
   // Promotable integer types are required to be promoted by the ABI.

clang/test/CodeGen/SystemZ/strictfp_builtins.c

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// REQUIRES: systemz-registered-target
+// RUN: %clang_cc1 %s -emit-llvm -ffp-exception-behavior=maytrap -o - -triple s390x-linux-gnu | FileCheck %s
+
+#pragma float_control(except, on)
+
+// CHECK-LABEL: @test_isnan_float(
+// 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:    [[TMP1:%.*]] = call i32 @llvm.s390.tdc.f32(float [[TMP0]], i64 15) [[ATTR2:#.*]]
+// CHECK-NEXT:    ret i32 [[TMP1]]
+//
+int test_isnan_float(float f) {
+  return __builtin_isnan(f);
+}
+
+// CHECK-LABEL: @test_isnan_double(
+// 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:    [[TMP1:%.*]] = call i32 @llvm.s390.tdc.f64(double [[TMP0]], i64 15) [[ATTR2]]
+// CHECK-NEXT:    ret i32 [[TMP1]]
+//
+int test_isnan_double(double d) {
+  return __builtin_isnan(d);
+}
+
+// CHECK-LABEL: @test_isnan_long_double(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[LD_ADDR:%.*]] = alloca fp128, align 8
+// CHECK-NEXT:    [[LD:%.*]] = load fp128, fp128* [[TMP0:%.*]], align 8
+// CHECK-NEXT:    store fp128 [[LD]], fp128* [[LD_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load fp128, fp128* [[LD_ADDR]], align 8
+// CHECK-NEXT:    [[TMP2:%.*]] = call i32 @llvm.s390.tdc.f128(fp128 [[TMP1]], i64 15) [[ATTR2]]
+// CHECK-NEXT:    ret i32 [[TMP2]]
+//
+int test_isnan_long_double(long double ld) {
+  return __builtin_isnan(ld);
+}
+