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

[Jonas Paulsson via Phabricator via cfe-commits]

jonpa added a comment.

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

thopre added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Paulsson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe57bd1ff4fb6: [CFE, SystemZ]  New target hook testFPKind() 
for checks of FP values. (authored by jonpa).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96568

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/SystemZ/strictfp_builtins.c

Index: clang/test/CodeGen/SystemZ/strictfp_builtins.c
===
--- /dev/null
+++ clang/test/CodeGen/SystemZ/strictfp_builtins.c
@@ -0,0 +1,43 @@
+// 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);
+}
+
Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -14,6 +14,7 @@
 #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"
@@ -126,6 +127,16 @@
 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 ,
+ CodeGenModule ) 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.
   ///
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #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"
@@ -30,6 +31,7 @@
 #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  // std::sort
@@ -7200,8 +7202,37 @@
   SystemZTargetCodeGenInfo(CodeGenTypes , bool HasVector, bool SoftFloatABI)
   : TargetCodeGenInfo(
 std::make_unique(CGT, HasVector, SoftFloatABI)) {}
-};
 
+  llvm::Value *testFPKind(llvm::Value *V, unsigned BuiltinID,
+  CGBuilderTy ,
+  CodeGenModule ) 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  = CGM.getModule();
+  auto  = M.getContext();
+  llvm::Function *TDCFunc =
+  llvm::Intrinsic::getDeclaration(, llvm::Intrinsic::s390_tdc, Ty);
+  

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

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

LGTM as well, thanks!


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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Paulsson via Phabricator via cfe-commits]

jonpa added a comment.

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?


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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

thopre added a comment.

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!


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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Paulsson via Phabricator via cfe-commits]

jonpa updated this revision to Diff 324376.
jonpa added a comment.

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


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

https://reviews.llvm.org/D96568

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/SystemZ/strictfp_builtins.c

Index: clang/test/CodeGen/SystemZ/strictfp_builtins.c
===
--- /dev/null
+++ clang/test/CodeGen/SystemZ/strictfp_builtins.c
@@ -0,0 +1,43 @@
+// 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);
+}
+
Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -14,6 +14,7 @@
 #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"
@@ -126,6 +127,16 @@
 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 ,
+ CodeGenModule ) 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.
   ///
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #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"
@@ -30,6 +31,7 @@
 #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  // std::sort
@@ -7200,8 +7202,37 @@
   SystemZTargetCodeGenInfo(CodeGenTypes , bool HasVector, bool SoftFloatABI)
   : TargetCodeGenInfo(
 std::make_unique(CGT, HasVector, SoftFloatABI)) {}
-};
 
+  llvm::Value *testFPKind(llvm::Value *V, unsigned BuiltinID,
+  CGBuilderTy ,
+  CodeGenModule ) 

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

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

thopre added a comment.

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() == .   
>>> 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.


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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Paulsson via Phabricator via cfe-commits]

jonpa added a comment.

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() == .   
>> 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?


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

https://reviews.llvm.org/D96568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits