clang: Fix handling of __builtin_elementwise_copysign

[arsenm]

Summary

I realized the handling of copysign made no sense at all.
Only the type of the first operand should really matter, and
it should not perform a conversion between them.

Also fixes misleading errors and producing broken IR for
integers.

We could accept different FP types for the sign argument,
if the intrinsic permitted different types like the DAG node.
As it is we would need to insert a cast, which could have
other effects (like trapping on snan) which should not happen
for a copysign.

[arsenm]

ping

[erichkeane]

1 nit, and 1 trying to see what is going on. I don't have a good feeling what the purpose of this builtin is, nor whether it matches the desire/intent of this builtin, I'm hopeful one of the other reviewers has the ability to check that.

clang/lib/Sema/SemaChecking.cpp

2048

you can just do "return S.Diag", which always returns 'true'. This will save a line, and the need for curleys.

2658

What is the point of the Unary Conversions here? Its a touch surprising to see a builtin do that? Note that it does quite a bit of FP related conversions, so are you sure you want those?

[arsenm]

In D140639#4028883, @erichkeane wrote:

1 nit, and 1 trying to see what is going on. I don't have a good feeling what the purpose of this builtin is,

The point of every builtin is direct access to llvm intrinsics, in this case llvm.copysign. I need it on vectors and not the set of 3 scalars it handles now.

clang/lib/Sema/SemaChecking.cpp

2658

Copied from the other elementwise intrinsics, and this is at the edges of my frontend knowledge (I guess it's to avoid qualifiers mattering?). The tests seem to behave as I expect

[erichkeane]

In D140639#4028900, @arsenm wrote:

In D140639#4028883, @erichkeane wrote:

1 nit, and 1 trying to see what is going on. I don't have a good feeling what the purpose of this builtin is,

The point of every builtin is direct access to llvm intrinsics, in this case llvm.copysign. I need it on vectors and not the set of 3 scalars it handles now.

I'm unfamiliar with the semantics of that builtin other than what i can read here: https://llvm.org/docs/LangRef.html#llvm-copysign-intrinsic

clang/lib/Sema/SemaChecking.cpp

2658

It really depends on what behavior you're looking to get out of this. UnaryConversions are usually for operators, not 'function like' things, so it is a touch jarring to me.

I guess I would have expected DefaultFunctionArrayLValueConversion (which calls DefaultLValueConversion after doing array-to-pointer conversions).

If the idea is for this builtin to act more like a variadic arg, I'd expect to see DefaultArgumentPromotion.

@aaron.ballman I think is smarter than me in regards to how these should work, so perhaps he can comment here?

I DO note one of the things that UsualUnaryConversions is doing is removing 'half' types on platforms without a 'native' half type. I would expect those sorts of conversions wouldn't be right?

2672

curleys not used for single-statement if-statement bodies.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

2658

I think we need to do the usual unary conversions because that's what handles the floating point evaluation method stuff, and if I'm reading this properly, it seems that copysign() does perform these conversions: https://godbolt.org/z/eWjEqvvjd. I would not expect to handle this with default argument promotion given the signature of copysign().

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

2658

Got it, thanks for the clarification.

[arsenm]

clang/lib/Sema/SemaChecking.cpp

2672

It covers 3 lines, it should have braces

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

2672

Our standard says 'statments' not 'lines'. This is a single statement.

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[arsenm]

clang/lib/Sema/SemaChecking.cpp

2672

Also says 'Similarly, braces should be used when a single-statement body is complex enough that it becomes difficult to see where the block containing the following statement began.'

Which is any case where it covers multiple lines

[arsenm]

clang/lib/Sema/SemaChecking.cpp

2672

Plus I'd go by clang-format does not add braces here

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

2672

I don't see the 'difficult to see when the following statement began' bit in this code, particularly with the new line.

By policy, I don't think we have curleys here. I'm of the opinion of 'almost always curleys', but our policy.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

2672

I think a strict reading of our policy is that the preference is to elide the braces in this case, but FWIW, I'm perfectly fine with leaving the braces in as they (ever-so-slightly) improve the readability of the code given that it spans multiple source lines.

[erichkeane]

I still want someone more familiar with LLVM to review this, but code wise I think we're ok (modulus 1 suggestion I made in checkFPMathBuiltinElementType).

clang/lib/Sema/SemaChecking.cpp

2672

Thanks, I really dislike that it is 'single statement' with some vague extensions, but not my decision :/

[arsenm]

Return S.Diag

[arsenm]

ping, I think this should get in before the branch date to fix the current broken behavior before this is in a release

[aaron.ballman]

LGTM!

[arsenm]

2ad4c3c88d884684a3efb42181e87fe305df51bd

clang/lib/Sema/SemaChecking.cpp

bool Sema::CheckTSBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
   case llvm::Triple::riscv64:
     return CheckRISCVBuiltinFunctionCall(TI, BuiltinID, TheCall);
   case llvm::Triple::loongarch32:
   case llvm::Triple::loongarch64:
     return CheckLoongArchBuiltinFunctionCall(TI, BuiltinID, TheCall);
   }
 }
 
+// Check if \p Ty is a valid type for the elementwise math builtins. If it is
+// not a valid type, emit an error message and return true. Otherwise return
+// false.
+static bool checkMathBuiltinElementType(Sema &S, SourceLocation Loc,
+                                        QualType Ty) {
+  if (!Ty->getAs<VectorType>() && !ConstantMatrixType::isValidElementType(Ty)) {
+    return S.Diag(Loc, diag::err_builtin_invalid_arg_type)
+           << 1 << /* vector, integer or float ty*/ 0 << Ty;
+  }
+
+  return false;
+}
+
+static bool checkFPMathBuiltinElementType(Sema &S, SourceLocation Loc,
+                                          QualType ArgTy, int ArgIndex) {
+  QualType EltTy = ArgTy;
+  if (auto *VecTy = EltTy->getAs<VectorType>())
+    EltTy = VecTy->getElementType();
+
+  if (!EltTy->isRealFloatingType()) {
+    return S.Diag(Loc, diag::err_builtin_invalid_arg_type)
+           << ArgIndex << /* vector or float ty*/ 5 << ArgTy;
+  }
+
+  return false;
+}
+
 ExprResult
 Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
                                CallExpr *TheCall) {
   ExprResult TheCallResult(TheCall);
 
   // Find out if any arguments are required to be integer constant expressions.
   unsigned ICEArguments = 0;
   ASTContext::GetBuiltinTypeError Error;
if (!EltTy->isIntegerType()) {
           << 1 << /* integer ty */ 6 << ArgTy;
       return ExprError();
     }
     break;
   }
 
   case Builtin::BI__builtin_elementwise_min:
   case Builtin::BI__builtin_elementwise_max:
-  case Builtin::BI__builtin_elementwise_copysign:
     if (SemaBuiltinElementwiseMath(TheCall))
       return ExprError();
     break;
+  case Builtin::BI__builtin_elementwise_copysign: {
+    if (checkArgCount(*this, TheCall, 2))
+      return ExprError();
+
+    ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0));
+    ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1));
+    if (Magnitude.isInvalid() || Sign.isInvalid())
+      return ExprError();
+
+    QualType MagnitudeTy = Magnitude.get()->getType();
+    QualType SignTy = Sign.get()->getType();
+    if (checkFPMathBuiltinElementType(*this, TheCall->getArg(0)->getBeginLoc(),
+                                      MagnitudeTy, 1) ||
+        checkFPMathBuiltinElementType(*this, TheCall->getArg(1)->getBeginLoc(),
+                                      SignTy, 2)) {
+      return ExprError();
+    }
+
+    if (MagnitudeTy.getCanonicalType() != SignTy.getCanonicalType()) {
+      return Diag(Sign.get()->getBeginLoc(),
+                  diag::err_typecheck_call_different_arg_types)
+             << MagnitudeTy << SignTy;
+    }
+
+    TheCall->setArg(0, Magnitude.get());
+    TheCall->setArg(1, Sign.get());
+    TheCall->setType(Magnitude.get()->getType());
+    break;
+  }
   case Builtin::BI__builtin_reduce_max:
   case Builtin::BI__builtin_reduce_min: {
     if (PrepareBuiltinReduceMathOneArgCall(TheCall))
       return ExprError();
 
     const Expr *Arg = TheCall->getArg(0);
     const auto *TyA = Arg->getType()->getAs<VectorType>();
     if (!TyA) {

 void Sema::CheckAddressOfPackedMember(Expr *rhs) {
   using namespace std::placeholders;
 
   RefersToMemberWithReducedAlignment(
       rhs, std::bind(&Sema::AddPotentialMisalignedMembers, std::ref(*this), _1,
                      _2, _3, _4));
 }
 
-// Check if \p Ty is a valid type for the elementwise math builtins. If it is
-// not a valid type, emit an error message and return true. Otherwise return
-// false.
-static bool checkMathBuiltinElementType(Sema &S, SourceLocation Loc,
-                                        QualType Ty) {
-  if (!Ty->getAs<VectorType>() && !ConstantMatrixType::isValidElementType(Ty)) {
-    S.Diag(Loc, diag::err_builtin_invalid_arg_type)
-        << 1 << /* vector, integer or float ty*/ 0 << Ty;
-    return true;
-  }
-  return false;
-}
-
 bool Sema::PrepareBuiltinElementwiseMathOneArgCall(CallExpr *TheCall) {
   if (checkArgCount(*this, TheCall, 1))
     return true;
 
   ExprResult A = UsualUnaryConversions(TheCall->getArg(0));
   if (A.isInvalid())
     return true;
 

clang/test/CodeGen/builtins-elementwise-math.c

void test_builtin_elementwise_canonicalize(float f1, float f2, double d1, double d2,
   d2 = __builtin_elementwise_canonicalize(d1);
 
   // CHECK:      [[VF1:%.+]] = load <4 x float>, ptr %vf1.addr, align 16
   // CHECK-NEXT: call <4 x float> @llvm.canonicalize.v4f32(<4 x float> [[VF1]])
   vf2 = __builtin_elementwise_canonicalize(vf1);
 }
 
 void test_builtin_elementwise_copysign(float f1, float f2, double d1, double d2,
-                                       float4 vf1, float4 vf2) {
+                                       float4 vf1, float4 vf2, double2 v2f64) {
   // CHECK-LABEL: define void @test_builtin_elementwise_copysign(
   // CHECK:      [[F1:%.+]] = load float, ptr %f1.addr, align 4
   // CHECK-NEXT: [[F2:%.+]] = load float, ptr %f2.addr, align 4
   // CHECK-NEXT:  call float @llvm.copysign.f32(float %0, float %1)
   f1 = __builtin_elementwise_copysign(f1, f2);
 
   // CHECK:      [[D1:%.+]] = load double, ptr %d1.addr, align 8
   // CHECK-NEXT: [[D2:%.+]] = load double, ptr %d2.addr, align 8
void test_builtin_elementwise_copysign(float f1, float f2, double d1, double d2,
   // CHECK-NEXT: call <4 x float> @llvm.copysign.v4f32(<4 x float> [[CVF1]], <4 x float> [[VF2]])
   const float4 cvf1 = vf1;
   vf1 = __builtin_elementwise_copysign(cvf1, vf2);
 
   // CHECK:      [[VF2:%.+]] = load <4 x float>, ptr %vf2.addr, align 16
   // CHECK-NEXT: [[CVF1:%.+]] = load <4 x float>, ptr %cvf1, align 16
   // CHECK-NEXT: call <4 x float> @llvm.copysign.v4f32(<4 x float> [[VF2]], <4 x float> [[CVF1]])
   vf1 = __builtin_elementwise_copysign(vf2, cvf1);
+
+
+  // CHECK:      [[F1:%.+]] = load float, ptr %f1.addr
+  // CHECK-NEXT: call float @llvm.copysign.f32(float [[F1]], float 2.000000e+00)
+  f1 = __builtin_elementwise_copysign(f1, 2.0f);
+
+  // CHECK:      [[F1:%.+]] = load float, ptr %f1.addr
+  // CHECK-NEXT: call float @llvm.copysign.f32(float 2.000000e+00, float [[F1]])
+  f1 = __builtin_elementwise_copysign(2.0f, f1);
+
+  // CHECK:      [[V2F64:%.+]] = load <2 x double>, ptr %v2f64.addr, align 16
+  // CHECK-NEXT: call <2 x double> @llvm.copysign.v2f64(<2 x double> <double 1.000000e+00, double 1.000000e+00>, <2 x double> [[V2F64]])
+  v2f64 = __builtin_elementwise_copysign((double2)1.0, v2f64);
 }

clang/test/Sema/builtins-elementwise-math.c

 // RUN: %clang_cc1 -std=c99 %s -pedantic -verify -triple=x86_64-apple-darwin9
 
+typedef double double2 __attribute__((ext_vector_type(2)));
+typedef double double4 __attribute__((ext_vector_type(4)));
+typedef float float2 __attribute__((ext_vector_type(2)));
 typedef float float4 __attribute__((ext_vector_type(4)));
 typedef int int3 __attribute__((ext_vector_type(3)));
 typedef unsigned unsigned3 __attribute__((ext_vector_type(3)));
 typedef unsigned unsigned4 __attribute__((ext_vector_type(4)));
 
 struct Foo {
   char *p;
 };
 
 __attribute__((address_space(1))) int int_as_one;
 typedef int bar;
 bar b;
 
+__attribute__((address_space(1))) float float_as_one;
+typedef float waffle;
+waffle waf;
+
+
 void test_builtin_elementwise_abs(int i, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
   struct Foo s = __builtin_elementwise_abs(i);
   // expected-error@-1 {{initializing 'struct Foo' with an expression of incompatible type 'int'}}
 
   i = __builtin_elementwise_abs();
   // expected-error@-1 {{too few arguments to function call, expected 1, have 0}}
 
   i = __builtin_elementwise_abs(i, i);
void test_builtin_elementwise_canonicalize(int i, float f, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
 
   u = __builtin_elementwise_canonicalize(u);
   // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned int')}}
 
   uv = __builtin_elementwise_canonicalize(uv);
   // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned4' (vector of 4 'unsigned int' values))}}
 }
 
-void test_builtin_elementwise_copysign(int i, short s, double d, float4 v, int3 iv, unsigned3 uv, int *p) {
+void test_builtin_elementwise_copysign(int i, short s, double d, float f, float4 v, int3 iv, unsigned3 uv, int *p) {
   i = __builtin_elementwise_copysign(p, d);
-  // expected-error@-1 {{arguments are of different types ('int *' vs 'double')}}
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int *')}}
 
-  struct Foo foo = __builtin_elementwise_copysign(i, i);
-  // expected-error@-1 {{initializing 'struct Foo' with an expression of incompatible type 'int'}}
+  i = __builtin_elementwise_copysign(i, i);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
 
   i = __builtin_elementwise_copysign(i);
   // expected-error@-1 {{too few arguments to function call, expected 2, have 1}}
 
   i = __builtin_elementwise_copysign();
   // expected-error@-1 {{too few arguments to function call, expected 2, have 0}}
 
   i = __builtin_elementwise_copysign(i, i, i);
   // expected-error@-1 {{too many arguments to function call, expected 2, have 3}}
 
   i = __builtin_elementwise_copysign(v, iv);
-  // expected-error@-1 {{arguments are of different types ('float4' (vector of 4 'float' values) vs 'int3' (vector of 3 'int' values))}}
+  // expected-error@-1 {{2nd argument must be a floating point type (was 'int3' (vector of 3 'int' values))}}
 
   i = __builtin_elementwise_copysign(uv, iv);
-  // expected-error@-1 {{arguments are of different types ('unsigned3' (vector of 3 'unsigned int' values) vs 'int3' (vector of 3 'int' values))}}
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned3' (vector of 3 'unsigned int' values))}}
 
   s = __builtin_elementwise_copysign(i, s);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
+
+  f = __builtin_elementwise_copysign(f, i);
+  // expected-error@-1 {{2nd argument must be a floating point type (was 'int')}}
+
+  f = __builtin_elementwise_copysign(i, f);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
 
   enum e { one,
            two };
   i = __builtin_elementwise_copysign(one, two);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
 
   enum f { three };
   enum f x = __builtin_elementwise_copysign(one, three);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
 
   _BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
   ext = __builtin_elementwise_copysign(ext, ext);
+  // expected-error@-1 {{1st argument must be a floating point type (was '_BitInt(32)')}}
 
-  const int ci;
-  i = __builtin_elementwise_copysign(ci, i);
-  i = __builtin_elementwise_copysign(i, ci);
-  i = __builtin_elementwise_copysign(ci, ci);
+  const float cf32;
+  f = __builtin_elementwise_copysign(cf32, f);
+  f = __builtin_elementwise_copysign(f, cf32);
+  f = __builtin_elementwise_copysign(cf32, f);
 
-  i = __builtin_elementwise_copysign(i, int_as_one); // ok (attributes don't match)?
-  i = __builtin_elementwise_copysign(i, b);          // ok (sugar doesn't match)?
+  f = __builtin_elementwise_copysign(f, float_as_one); // ok (attributes don't match)?
+  f = __builtin_elementwise_copysign(f, waf);          // ok (sugar doesn't match)?
 
-  int A[10];
+  float A[10];
   A = __builtin_elementwise_copysign(A, A);
-  // expected-error@-1 {{1st argument must be a vector, integer or floating point type (was 'int *')}}
+  // expected-error@-1 {{1st argument must be a floating point type (was 'float *')}}
 
-  int(ii);
-  int j;
-  j = __builtin_elementwise_copysign(i, j);
+  float(ii);
+  float j;
+  j = __builtin_elementwise_copysign(f, j);
 
   _Complex float c1, c2;
   c1 = __builtin_elementwise_copysign(c1, c2);
-  // expected-error@-1 {{1st argument must be a vector, integer or floating point type (was '_Complex float')}}
+  // expected-error@-1 {{1st argument must be a floating point type (was '_Complex float')}}
+
+  double f64 = 0.0;
+  double tmp0 = __builtin_elementwise_copysign(f64, f);
+  // expected-error@-1 {{arguments are of different types ('double' vs 'float')}}
+
+  float tmp1 = __builtin_elementwise_copysign(f, f64);
+  //expected-error@-1 {{arguments are of different types ('float' vs 'double')}}
+
+  float4 v4f32 = 0.0f;
+  float4 tmp2 = __builtin_elementwise_copysign(v4f32, f);
+  // expected-error@-1 {{arguments are of different types ('float4' (vector of 4 'float' values) vs 'float')}}
+
+  float tmp3 = __builtin_elementwise_copysign(f, v4f32);
+  // expected-error@-1 {{arguments are of different types ('float' vs 'float4' (vector of 4 'float' values))}}
+
+  float2 v2f32 = 0.0f;
+  double4 v4f64 = 0.0;
+  double4 tmp4 = __builtin_elementwise_copysign(v4f64, v4f32);
+  // expected-error@-1 {{arguments are of different types ('double4' (vector of 4 'double' values) vs 'float4' (vector of 4 'float' values))}}
+
+  float4 tmp6 = __builtin_elementwise_copysign(v4f32, v4f64);
+  // expected-error@-1 {{arguments are of different types ('float4' (vector of 4 'float' values) vs 'double4' (vector of 4 'double' values))}}
+
+  float4 tmp7 = __builtin_elementwise_copysign(v4f32, v2f32);
+  // expected-error@-1 {{arguments are of different types ('float4' (vector of 4 'float' values) vs 'float2' (vector of 2 'float' values))}}
+
+  float2 tmp8 = __builtin_elementwise_copysign(v2f32, v4f32);
+  // expected-error@-1 {{arguments are of different types ('float2' (vector of 2 'float' values) vs 'float4' (vector of 4 'float' values))}}
+
+  float2 tmp9 = __builtin_elementwise_copysign(v4f32, v4f32);
+  // expected-error@-1 {{initializing 'float2' (vector of 2 'float' values) with an expression of incompatible type 'float4' (vector of 4 'float' values)}}
 }