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
bsaleil
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.Sep 17 2020, 12:24 PM

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

Conanap marked an inline comment as done.Sep 17 2020, 12:25 PM
bsaleil added inline comments.Sep 23 2020, 11:37 AM
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 requested changes to this revision.Sep 24 2020, 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.Sep 24 2020, 4:34 AM
Conanap updated this revision to Diff 294778.Sep 28 2020, 12:24 PM
Conanap marked an inline comment as done.

Changed the location of fix and added extra test cases

bsaleil accepted this revision.Mon, Oct 5, 2:59 PM

LGTM, please fix the unrelated change when committing.

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

Unrelated change ?

nemanjai added inline comments.Wed, Oct 7, 5:29 AM
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 updated this revision to Diff 297936.Tue, Oct 13, 1:06 PM

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

Conanap marked 3 inline comments as done.Tue, Oct 13, 1:07 PM

Addressed Nemenja's concerns.