Page MenuHomePhabricator

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

Authored by Conanap on Jul 30 2020, 11:28 AM.

Details

Reviewers
saghir
nemanjai
hfinkel
Group Reviewers
Restricted Project
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

Diff Detail

Event Timeline

Conanap created this revision.Jul 30 2020, 11:28 AM
Conanap requested review of this revision.Jul 30 2020, 11:28 AM
Conanap edited the summary of this revision. (Show Details)Jul 30 2020, 11:31 AM
Conanap added a subscriber: llvm-commits.
bsaleil added inline comments.
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 ?

Yes, this definitely needs a test case.

Conanap updated this revision to Diff 292589.Thu, Sep 17, 12:24 PM

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

Conanap marked an inline comment as done.Thu, Sep 17, 12:25 PM
bsaleil added inline comments.Wed, Sep 23, 11:37 AM
clang/test/CodeGen/builtins-ppc-vsx.c
1840

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 requested changes to this revision.Thu, Sep 24, 4:34 AM

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.

This revision now requires changes to proceed.Thu, Sep 24, 4:34 AM
Conanap updated this revision to Diff 294778.Mon, Sep 28, 12:24 PM
Conanap marked an inline comment as done.

Changed the location of fix and added extra test cases