[PowerPC] Correct cpsgn's behaviour on PowerPC to match that of the ABI

[Conanap]

Summary

If VC = cpsgn(VA, VB),

Current behaviour:

VA = -1.000000000 2.000000000 -3.000000000 4.000000000
VB = 1.000000000 1.000000000 1.000000000 1.000000000
VC = 1.000000000 2.000000000 3.000000000 4.000000000

Expected behaviour (as per ABI document):

VA -1.000000000 2.000000000 -3.000000000 4.000000000
VB 1.000000000 1.000000000 1.000000000 1.000000000
VC -1.000000000 1.000000000 -1.000000000 1.000000000

[bsaleil]

clang/lib/CodeGen/CGBuiltin.cpp

14298 ↗

(On Diff #281994)

Could you add a test case in clang/test/CodeGen/builtins-ppc-vsx.c showing that calls to the builtins and calls to vec_cpsgn are generated as calls to the copysign LLVM intrinsic with the arguments being inverted ?

[nemanjai]

Yes, this definitely needs a test case.

[Conanap]

Added extra test case to demonstrate that the arguments will be reversed

[bsaleil]

clang/test/CodeGen/builtins-ppc-vsx.c

1858

Could you also add a call to __builtin_vsx_xvcpsgnsp and the same tests for vector double, so with calls to __builtin_vsx_xvcpsgndp and vec_cpsgn with vector double arguments ?

[nemanjai]

This is not what we want. The builtin behaves correctly. It is equivalent to the generic __builtin_copysign and it would be very surprising to a user if it reverses the operands depending on which version you use. What is implemented incorrectly is vec_cpsgn so that's what should change.

[Conanap]

Changed the location of fix and added extra test cases

[bsaleil]

LGTM, please fix the unrelated change when committing.

clang/test/CodeGen/builtins-ppc-vsx.c

1

Unrelated change ?

[nemanjai]

clang/test/CodeGen/builtins-ppc-vsx.c

1

I am not sure if this even works or not, but no other tests have it in lowercase and neither should this one. In addition of course to it being an unrelated change.

1856

The test case should not hard-code the virtual register names such as %4, %5, %6. Those should be set in the signature of the function and checked in the call instruction. I believe that vreg naming is different on non-assert builds which would make this fail right away on bots that build without asserts/debug.

[Conanap]

Use regex match to check for variable order in IR instead.

[Conanap]

Addressed Nemenja's concerns.

[nemanjai]

LGTM.

clang/lib/Headers/altivec.h

 #endif
 #endif
 
 /* vec_cpsgn */
 
 #ifdef __VSX__
 static __inline__ vector float __ATTRS_o_ai vec_cpsgn(vector float __a,
                                                       vector float __b) {
-  return __builtin_vsx_xvcpsgnsp(__a, __b);
+  return __builtin_vsx_xvcpsgnsp(__b, __a);
 }
 
 static __inline__ vector double __ATTRS_o_ai vec_cpsgn(vector double __a,
                                                        vector double __b) {
-  return __builtin_vsx_xvcpsgndp(__a, __b);
+  return __builtin_vsx_xvcpsgndp(__b, __a);
 }
 #endif
 
 /* vec_ctf */
 
 #ifdef __VSX__
 #define vec_ctf(__a, __b)                                                      \
   _Generic((__a), vector int                                                   \

clang/test/CodeGen/builtins-ppc-vsx.c

 // REQUIRES: powerpc-registered-target
 // RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 // RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-LE
 #include <altivec.h>
 
 vector bool char vbc = { 0, 1, 0, 1, 0, 1, 0, 1,
                          0, 1, 0, 1, 0, 1, 0, 1 };
 vector signed char vsc = { -8,  9, -10, 11, -12, 13, -14, 15,
                            -0,  1,  -2,  3,  -4,  5,  -6,  7};
// CHECK-NEXT-LE: %{{[0-9]+}} = extractelement <2 x i64> %[[V1]], i32 1
   __builtin_unpack_vector_int128(res_vslll, 1);
 // CHECK:  %[[V1:[0-9]+]] = bitcast <1 x i128> %{{[0-9]+}} to <2 x i64>
 // CHECK-NEXT: %{{[0-9]+}} = extractelement <2 x i64> %[[V1]], i32 1
 
 // CHECK-LE:  %[[V1:[0-9]+]] = bitcast <1 x i128> %{{[0-9]+}} to <2 x i64>
 // CHECK-NEXT-LE: %{{[0-9]+}} = extractelement <2 x i64> %[[V1]], i32 0
 
 }
+
+void test_vector_cpsgn_float(vector float a, vector float b) {
+// CHECK-LABEL: test_vector_cpsgn_float
+// CHECK-DAG: load{{.*}}%__a
+// CHECK-DAG: load{{.*}}%__b
+// CHECK-NOT: SEPARATOR
+// CHECK-DAG: [[RA:%[0-9]+]] = load <4 x float>, <4 x float>* %__a.addr
+// CHECK-DAG: [[RB:%[0-9]+]] = load <4 x float>, <4 x float>* %__b.addr
+// CHECK-NEXT: call <4 x float> @llvm.copysign.v4f32(<4 x float> [[RB]], <4 x float> [[RA]])
+  vec_cpsgn(a, b);
+}
+
+void test_vector_cpsgn_double(vector double a, vector double b) {
+// CHECK-LABEL: test_vector_cpsgn_double
+// CHECK-DAG: load{{.*}}%__a
+// CHECK-DAG: load{{.*}}%__b
+// CHECK-NOT: SEPARATOR
+// CHECK-DAG: [[RA:%[0-9]+]] = load <2 x double>, <2 x double>* %__a.addr
+// CHECK-DAG: [[RB:%[0-9]+]] = load <2 x double>, <2 x double>* %__b.addr
+// CHECK-NEXT: call <2 x double> @llvm.copysign.v2f64(<2 x double> [[RB]], <2 x double> [[RA]])
+  vec_cpsgn(a, b);
+}
+
+void test_builtin_xvcpsgnsp(vector float a, vector float b) {
+// CHECK-LABEL: test_builtin_xvcpsgnsp
+// CHECK-DAG: load{{.*}}%a
+// CHECK-DAG: load{{.*}}%b
+// CHECK-NOT: SEPARATOR
+// CHECK-DAG: [[RA:%[0-9]+]] = load <4 x float>, <4 x float>* %a.addr
+// CHECK-DAG: [[RB:%[0-9]+]] = load <4 x float>, <4 x float>* %b.addr
+// CHECK-NEXT: call <4 x float> @llvm.copysign.v4f32(<4 x float> [[RA]], <4 x float> [[RB]])
+  __builtin_vsx_xvcpsgnsp(a, b);
+}
+
+void test_builtin_xvcpsgndp(vector double a, vector double b) {
+// CHECK-LABEL: test_builtin_xvcpsgndp
+// CHECK-DAG: load{{.*}}%a
+// CHECK-DAG: load{{.*}}%b
+// CHECK-NOT: SEPARATOR
+// CHECK-DAG: [[RA:%[0-9]+]] = load <2 x double>, <2 x double>* %a.addr
+// CHECK-DAG: [[RB:%[0-9]+]] = load <2 x double>, <2 x double>* %b.addr
+// CHECK-NEXT: call <2 x double> @llvm.copysign.v2f64(<2 x double> [[RA]], <2 x double> [[RB]])
+  __builtin_vsx_xvcpsgndp(a, b);
+}