address PR comments - removed faceforward_vec_impl, only check first arg is float in sema spirv checks, add half tests to codegen spirv test

Author: kmpeng

clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h

@@ -127,16 +127,6 @@ template <typename T> constexpr vector<T, 4> lit_impl(T NDotL, T NDotH, T M) {
 }
 
 template <typename T> constexpr T faceforward_impl(T N, T I, T Ng) {
-#if (__has_builtin(__builtin_spirv_faceforward))
-  return __builtin_spirv_faceforward(N, I, Ng);
-#else
-  return select(I * Ng < 0, N, -N);
-#endif
-}
-
-template <typename T, int L>
-constexpr vector<T, L> faceforward_vec_impl(vector<T, L> N, vector<T, L> I,
-                                            vector<T, L> Ng) {
 #if (__has_builtin(__builtin_spirv_faceforward))
   return __builtin_spirv_faceforward(N, I, Ng);
 #else

clang/lib/Headers/hlsl/hlsl_intrinsics.h

@@ -250,15 +250,15 @@ const inline __detail::HLSL_FIXED_VECTOR<half, L> faceforward(
     __detail::HLSL_FIXED_VECTOR<half, L> N,
     __detail::HLSL_FIXED_VECTOR<half, L> I,
     __detail::HLSL_FIXED_VECTOR<half, L> Ng) {
-  return __detail::faceforward_vec_impl(N, I, Ng);
+  return __detail::faceforward_impl(N, I, Ng);
 }
 
 template <int L>
 const inline __detail::HLSL_FIXED_VECTOR<float, L>
 faceforward(__detail::HLSL_FIXED_VECTOR<float, L> N,
             __detail::HLSL_FIXED_VECTOR<float, L> I,
             __detail::HLSL_FIXED_VECTOR<float, L> Ng) {
-  return __detail::faceforward_vec_impl(N, I, Ng);
+  return __detail::faceforward_impl(N, I, Ng);
 }
 
 //===----------------------------------------------------------------------===//

clang/lib/Sema/SemaSPIRV.cpp

@@ -16,20 +16,6 @@ namespace clang {
 
 SemaSPIRV::SemaSPIRV(Sema &S) : SemaBase(S) {}
 
-static bool CheckAllArgsHaveFloatRepresentation(Sema *S, CallExpr *TheCall) {
-  for (unsigned I = 0, N = TheCall->getNumArgs(); I < N; ++I) {
-    ExprResult Arg = TheCall->getArg(I);
-    QualType ArgTy = Arg.get()->getType();
-    if (!ArgTy->hasFloatingRepresentation()) {
-      S->Diag(Arg.get()->getBeginLoc(), diag::err_builtin_invalid_arg_type)
-          << /* ordinal */ I + 1 << /* scalar or vector */ 5 << /* no int */ 0
-          << /* fp */ 1 << ArgTy;
-      return true;
-    }
-  }
-  return false;
-}
-
 static bool CheckAllArgsHaveSameType(Sema *S, CallExpr *TheCall) {
   assert(TheCall->getNumArgs() > 1);
   QualType ArgTy0 = TheCall->getArg(0)->getType();
@@ -136,27 +122,41 @@ bool SemaSPIRV::CheckSPIRVBuiltinFunctionCall(unsigned BuiltinID,
     if (SemaRef.checkArgCount(TheCall, 3))
       return true;
 
-    if (CheckAllArgsHaveFloatRepresentation(&SemaRef, TheCall))
+    // Check if first argument has floating representation
+    ExprResult A = TheCall->getArg(0);
+    QualType ArgTyA = A.get()->getType();
+    if (!ArgTyA->hasFloatingRepresentation()) {
+      SemaRef.Diag(A.get()->getBeginLoc(), diag::err_builtin_invalid_arg_type)
+          << /* ordinal */ 1 << /* scalar or vector */ 5 << /* no int */ 0
+          << /* fp */ 1 << ArgTyA;
       return true;
+    }
 
     if (CheckAllArgsHaveSameType(&SemaRef, TheCall))
       return true;
 
-    QualType RetTy = TheCall->getArg(0)->getType();
+    QualType RetTy = ArgTyA;
     TheCall->setType(RetTy);
     break;
   }
   case SPIRV::BI__builtin_spirv_faceforward: {
     if (SemaRef.checkArgCount(TheCall, 3))
       return true;
 
-    if (CheckAllArgsHaveFloatRepresentation(&SemaRef, TheCall))
+    // Check if first argument has floating representation
+    ExprResult A = TheCall->getArg(0);
+    QualType ArgTyA = A.get()->getType();
+    if (!ArgTyA->hasFloatingRepresentation()) {
+      SemaRef.Diag(A.get()->getBeginLoc(), diag::err_builtin_invalid_arg_type)
+          << /* ordinal */ 1 << /* scalar or vector */ 5 << /* no int */ 0
+          << /* fp */ 1 << ArgTyA;
       return true;
+    }
 
     if (CheckAllArgsHaveSameType(&SemaRef, TheCall))
       return true;
 
-    QualType RetTy = TheCall->getArg(0)->getType();
+    QualType RetTy = ArgTyA;
     TheCall->setType(RetTy);
     break;
   }

clang/test/CodeGenHLSL/builtins/faceforward.hlsl

@@ -6,8 +6,8 @@
 // RUN:   -emit-llvm -o - | FileCheck %s --check-prefix=SPVCHECK
 
 // CHECK-LABEL: test_faceforward_half
-// CHECK: %mul.i = fmul reassoc nnan ninf nsz arcp afn half %{{.*}}, %{{.*}}
-// CHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %mul.i, 0xH0000
+// CHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn half %{{.*}}, %{{.*}}
+// CHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
 // CHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn half %{{.*}}
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
 // CHECK: ret half %hlsl.select.i
@@ -50,8 +50,8 @@ half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N,
 half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float
-// CHECK: %mul.i = fmul reassoc nnan ninf nsz arcp afn float %{{.*}}, %{{.*}}
-// CHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %mul.i, 0.000000e+00
+// CHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn float %{{.*}}, %{{.*}}
+// CHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
 // CHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn float %{{.*}}
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
 // CHECK: ret float %hlsl.select.i

clang/test/CodeGenSPIRV/Builtins/faceforward.c

@@ -2,12 +2,44 @@
 
 // RUN: %clang_cc1 -O1 -triple spirv-pc-vulkan-compute %s -emit-llvm -o - | FileCheck %s
 
+typedef _Float16 half;
+typedef half half2 __attribute__((ext_vector_type(2)));
+typedef half half3 __attribute__((ext_vector_type(3)));
+typedef half half4 __attribute__((ext_vector_type(4)));
 typedef float float2 __attribute__((ext_vector_type(2)));
 typedef float float3 __attribute__((ext_vector_type(3)));
 typedef float float4 __attribute__((ext_vector_type(4)));
 
+// CHECK-LABEL: define spir_func half @test_faceforward_half(
+// CHECK-SAME: half noundef [[N:%.*]], half noundef [[I:%.*]], half noundef [[NG:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[SPV_FACEFORWARD:%.*]] = tail call half @llvm.spv.smoothstep.f16(half [[N]], half [[I]], half [[NG]])
+// CHECK-NEXT:    ret half [[SPV_FACEFORWARD]]
+half test_faceforward_half(half N, half I, half Ng) { return __builtin_spirv_faceforward(N, I, Ng); }
+
+// CHECK-LABEL: define spir_func <2 x half> @test_faceforward_half2(
+// CHECK-SAME: <2 x half> noundef [[N:%.*]], <2 x half> noundef [[I:%.*]], <2 x half> noundef [[NG:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[SPV_FACEFORWARD:%.*]] = tail call <2 x half> @llvm.spv.smoothstep.v2f16(<2 x half> [[N]], <2 x half> [[I]], <2 x half> [[NG]])
+// CHECK-NEXT:    ret <2 x half> [[SPV_FACEFORWARD]]
+half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return __builtin_spirv_faceforward(N, I, Ng); }
+
+// CHECK-LABEL: define spir_func <3 x half> @test_faceforward_half3(
+// CHECK-SAME: <3 x half> noundef [[N:%.*]], <3 x half> noundef [[I:%.*]], <3 x half> noundef [[NG:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[SPV_FACEFORWARD:%.*]] = tail call <3 x half> @llvm.spv.smoothstep.v3f16(<3 x half> [[N]], <3 x half> [[I]], <3 x half> [[NG]])
+// CHECK-NEXT:    ret <3 x half> [[SPV_FACEFORWARD]]
+half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return __builtin_spirv_faceforward(N, I, Ng); }
+
+// CHECK-LABEL: define spir_func <4 x half> @test_faceforward_half4(
+// CHECK-SAME: <4 x half> noundef [[N:%.*]], <4 x half> noundef [[I:%.*]], <4 x half> noundef [[NG:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[SPV_FACEFORWARD:%.*]] = tail call <4 x half> @llvm.spv.smoothstep.v4f16(<4 x half> [[N]], <4 x half> [[I]], <4 x half> [[NG]])
+// CHECK-NEXT:    ret <4 x half> [[SPV_FACEFORWARD]]
+half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return __builtin_spirv_faceforward(N, I, Ng); }
+
 // CHECK-LABEL: define spir_func float @test_faceforward_float(
-// CHECK-SAME: float noundef [[N:%.*]], float noundef [[I:%.*]], float noundef [[NG:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-SAME: float noundef [[N:%.*]], float noundef [[I:%.*]], float noundef [[NG:%.*]]) local_unnamed_addr #[[ATTR0]] {
 // CHECK-NEXT:  [[ENTRY:.*:]]
 // CHECK-NEXT:    [[SPV_FACEFORWARD:%.*]] = tail call float @llvm.spv.smoothstep.f32(float [[N]], float [[I]], float [[NG]])
 // CHECK-NEXT:    ret float [[SPV_FACEFORWARD]]

clang/test/SemaSPIRV/BuiltIns/faceforward-errors.c

@@ -25,12 +25,12 @@ int test_int_scalar_inputs(int p0) {
 
 float test_int_scalar_inputs2(float p0, int p1) {
   return __builtin_spirv_faceforward(p0, p1, p1);
-  //  expected-error@-1 {{2nd argument must be a scalar or vector of floating-point types (was 'int')}}
+  //  expected-error@-1 {{all arguments to '__builtin_spirv_faceforward' must have the same type}}
 }
 
 float test_int_scalar_inputs3(float p0, int p1) {
   return __builtin_spirv_faceforward(p0, p0, p1);
-  //  expected-error@-1 {{3rd argument must be a scalar or vector of floating-point types (was 'int')}}
+  //  expected-error@-1 {{all arguments to '__builtin_spirv_faceforward' must have the same type}}
 }
 
 float test_mismatched_arg(float p0, float2 p1) {

clang/test/SemaSPIRV/BuiltIns/smoothstep-errors.c

@@ -25,12 +25,12 @@ int test_int_scalar_inputs(int p0) {
 
 float test_int_scalar_inputs2(float p0, int p1) {
   return __builtin_spirv_smoothstep(p0, p1, p1);
-  //  expected-error@-1 {{2nd argument must be a scalar or vector of floating-point types (was 'int')}}
+  //  expected-error@-1 {{all arguments to '__builtin_spirv_smoothstep' must have the same type}}
 }
 
 float test_int_scalar_inputs3(float p0, int p1) {
   return __builtin_spirv_smoothstep(p0, p0, p1);
-  //  expected-error@-1 {{3rd argument must be a scalar or vector of floating-point types (was 'int')}}
+  //  expected-error@-1 {{all arguments to '__builtin_spirv_smoothstep' must have the same type}}
 }
 
 float test_mismatched_arg(float p0, float2 p1) {

llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll

@@ -1,6 +1,9 @@
 ; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
 ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val --target-env vulkan1.3 %}
 
+; TODO(#136344): This test currently fails when --target-env vulkan1.3 is specified.
+; XFAIL: spirv-tools
+
 ; Make sure SPIRV operation function calls for faceforward are lowered correctly.
 
 ; CHECK-DAG: %[[#op_ext_glsl:]] = OpExtInstImport "GLSL.std.450"