[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

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

1840 ↗

(On Diff #292589)

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 ↗

(On Diff #294778)

Unrelated change ?

[nemanjai]

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

1 ↗

(On Diff #294778)

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.

1838 ↗

(On Diff #294778)

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/CodeGen/CGBuiltin.cpp

Value *CodeGenFunction::EmitPPCBuiltinExpr(unsigned BuiltinID,
   // Copy sign
   case PPC::BI__builtin_vsx_xvcpsgnsp:
   case PPC::BI__builtin_vsx_xvcpsgndp: {
     llvm::Type *ResultType = ConvertType(E->getType());
     Value *X = EmitScalarExpr(E->getArg(0));
     Value *Y = EmitScalarExpr(E->getArg(1));
     ID = Intrinsic::copysign;
     llvm::Function *F = CGM.getIntrinsic(ID, ResultType);
-    return Builder.CreateCall(F, {X, Y});
+    return Builder.CreateCall(F, {Y, X});
   }
   // Rounding/truncation
   case PPC::BI__builtin_vsx_xvrspip:
   case PPC::BI__builtin_vsx_xvrdpip:
   case PPC::BI__builtin_vsx_xvrdpim:
   case PPC::BI__builtin_vsx_xvrspim:
   case PPC::BI__builtin_vsx_xvrdpi:
   case PPC::BI__builtin_vsx_xvrspi: