address PR comments - move repeated code to helper function, remove -x specification in tests, clarify faceforward desc

Author: kmpeng

clang/lib/Headers/hlsl/hlsl_intrinsics.h

@@ -220,7 +220,7 @@ const inline double4 dst(double4 Src0, double4 Src1) {
 
 /// \fn T faceforward(T N, T I, T Ng)
 /// \brief Flips the surface-normal (if needed) to face in a direction opposite
-/// to \a I. Returns the result in \a N.
+/// to \a I. Returns the result in terms of \a N.
 /// \param N The resulting floating-point surface-normal vector.
 /// \param I A floating-point, incident vector that points from the view
 /// position to the shading position.

clang/lib/Sema/SemaSPIRV.cpp

@@ -16,6 +16,37 @@ 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();
+
+  for (unsigned I = 1, N = TheCall->getNumArgs(); I < N; ++I) {
+    if (!S->getASTContext().hasSameUnqualifiedType(
+            ArgTy0, TheCall->getArg(I)->getType())) {
+      S->Diag(TheCall->getBeginLoc(), diag::err_vec_builtin_incompatible_vector)
+          << TheCall->getDirectCallee() << /*useAllTerminology*/ true
+          << SourceRange(TheCall->getArg(0)->getBeginLoc(),
+                         TheCall->getArg(N - 1)->getEndLoc());
+      return true;
+    }
+  }
+  return false;
+}
+
 bool SemaSPIRV::CheckSPIRVBuiltinFunctionCall(unsigned BuiltinID,
                                               CallExpr *TheCall) {
   switch (BuiltinID) {
@@ -105,71 +136,27 @@ bool SemaSPIRV::CheckSPIRVBuiltinFunctionCall(unsigned BuiltinID,
     if (SemaRef.checkArgCount(TheCall, 3))
       return true;
 
-    // check if the all arguments have floating representation
-    for (unsigned i = 0; i < TheCall->getNumArgs(); ++i) {
-      ExprResult Arg = TheCall->getArg(i);
-      QualType ArgTy = Arg.get()->getType();
-      if (!ArgTy->hasFloatingRepresentation()) {
-        SemaRef.Diag(Arg.get()->getBeginLoc(),
-                     diag::err_builtin_invalid_arg_type)
-            << i + 1 << /* scalar or vector */ 5 << /* no int */ 0 << /* fp */ 1
-            << ArgTy;
-        return true;
-      }
-    }
+    if (CheckAllArgsHaveFloatRepresentation(&SemaRef, TheCall))
+      return true;
 
-    // check if all arguments are of the same type
-    ExprResult A = TheCall->getArg(0);
-    ExprResult B = TheCall->getArg(1);
-    ExprResult C = TheCall->getArg(2);
-    if (!(SemaRef.getASTContext().hasSameUnqualifiedType(A.get()->getType(),
-                                                         B.get()->getType()) &&
-          SemaRef.getASTContext().hasSameUnqualifiedType(A.get()->getType(),
-                                                         C.get()->getType()))) {
-      SemaRef.Diag(TheCall->getBeginLoc(),
-                   diag::err_vec_builtin_incompatible_vector)
-          << TheCall->getDirectCallee() << /*useAllTerminology*/ true
-          << SourceRange(A.get()->getBeginLoc(), C.get()->getEndLoc());
+    if (CheckAllArgsHaveSameType(&SemaRef, TheCall))
       return true;
-    }
 
-    QualType RetTy = A.get()->getType();
+    QualType RetTy = TheCall->getArg(0)->getType();
     TheCall->setType(RetTy);
     break;
   }
   case SPIRV::BI__builtin_spirv_faceforward: {
     if (SemaRef.checkArgCount(TheCall, 3))
       return true;
 
-    // check if all arguments have floating representation
-    for (unsigned i = 0; i < TheCall->getNumArgs(); ++i) {
-      ExprResult Arg = TheCall->getArg(i);
-      QualType ArgTy = Arg.get()->getType();
-      if (!ArgTy->hasFloatingRepresentation()) {
-        SemaRef.Diag(Arg.get()->getBeginLoc(),
-                     diag::err_builtin_invalid_arg_type)
-            << i + 1 << /* scalar or vector */ 5 << /* no int */ 0 << /* fp */ 1
-            << ArgTy;
-        return true;
-      }
-    }
+    if (CheckAllArgsHaveFloatRepresentation(&SemaRef, TheCall))
+      return true;
 
-    // check if all arguments are of the same type
-    ExprResult A = TheCall->getArg(0);
-    ExprResult B = TheCall->getArg(1);
-    ExprResult C = TheCall->getArg(2);
-    if (!(SemaRef.getASTContext().hasSameUnqualifiedType(A.get()->getType(),
-                                                         B.get()->getType()) &&
-          SemaRef.getASTContext().hasSameUnqualifiedType(A.get()->getType(),
-                                                         C.get()->getType()))) {
-      SemaRef.Diag(TheCall->getBeginLoc(),
-                   diag::err_vec_builtin_incompatible_vector)
-          << TheCall->getDirectCallee() << /*useAllTerminology*/ true
-          << SourceRange(A.get()->getBeginLoc(), C.get()->getEndLoc());
+    if (CheckAllArgsHaveSameType(&SemaRef, TheCall))
       return true;
-    }
 
-    QualType RetTy = A.get()->getType();
+    QualType RetTy = TheCall->getArg(0)->getType();
     TheCall->setType(RetTy);
     break;
   }

clang/test/CodeGenHLSL/builtins/faceforward.hlsl

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple \
+// RUN: %clang_cc1 -finclude-default-header -triple \
 // RUN:   dxil-pc-shadermodel6.3-library %s -fnative-half-type \
 // RUN:   -emit-llvm -o - | FileCheck %s
 // RUN: %clang_cc1 -finclude-default-header -triple \