Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vec_copysign implementations per issue #158, Part 1B. #161

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

munroesj52
Copy link
Contributor

Rebased after the add/subqpo merge. Cliose #158
Seems the GCC (and Clang followed) initially reversed the operands
for vec_cpsgn() (so the sign is copied from operand b into a).
This differs from the Intrinsic reference and ISA that say the sign
is copied from operand a into b.

Unfortunately PVECLIB implementations of vec_copysignf32(),
vec_copysignf64(), and vec_copysignf128() duplicated the results of
original GCC vec_cpsgn() for the implementation,

This has been reported as a bug and now GCC and Clang are in the
process of "fixing" this bug to match the Intrinsic reference guide.
The fix will be applied to currently supported versions but older
compiler version will remain unchanged.

This change set will correct the PVECLIB copysign implementation
to match the intrinsic reference manual. We will the macro
PVECLIB_CPSGN_FIXED to isolate the PVECLIB implementations from
the compiler changes.

* src/pveclib/vec_f128_ppc.h (vec_copysignf128): Change to
match operand order from Vector Intrinsic Reference.
(vec_xscvsqqp [_ARCH_PWR9]): Use correct vec_copysignf128
operand order.
* src/pveclib/vec_f32_ppc.h (vec_copysignf32): Change to match
operand order from Vector Intrinsic Reference.
* src/pveclib/vec_f64_ppc.h (vec_copysignf64): Change to match
operand order from Vector Intrinsic Reference.

* src/testsuite/arith128_test_f128.c (test_copysignf128):
Define new __float128 const f128_nsnan.
Reverse test operands and results to match corrected
vec_copysignf128() implementation. Remove duplicated tests.
* src/testsuite/arith128_test_f32.c (test_float_cpsgn):
Reverse test operands and results to match corrected
vec_copysignf32() implementation.
* src/testsuite/arith128_test_f64.c (test_double_cpsgn):
Reverse test operands and results to match corrected
vec_copysignf64() implementation.

* src/testsuite/vec_f32_dummy.c (test_vec_copysignf32):
new compile test.
* src/testsuite/vec_f64_dummy.c (test_vec_copysignf64):
new compile test.

Signed-off-by: Steven Munroe munroesj52@gmail.com

Rebased after the add/subqpo merge.
Seems the GCC (and Clang followed) initially reversed the operands
for vec_cpsgn() (so the sign is copied from operand b into a).
This differs from the Intrinsic reference and ISA that say the sign
is copied from operand a into b.

Unfortunately PVECLIB implementations of vec_copysignf32(),
vec_copysignf64(), and vec_copysignf128() duplicated the results of
original GCC vec_cpsgn() for the implementation,

This has been reported as a bug and now GCC and Clang are in the
process of "fixing" this bug to match the Intrinsic reference guide.
The fix will be applied to currently supported versions but older
compiler version will remain unchanged.

This change set will correct the PVECLIB copysign implementation
to match the intrinsic reference manual. We will the macro
PVECLIB_CPSGN_FIXED to isolate the PVECLIB implementations from
the compiler changes.

	* src/pveclib/vec_f128_ppc.h (vec_copysignf128): Change to
	match operand order from Vector Intrinsic Reference.
	(vec_xscvsqqp [_ARCH_PWR9]): Use correct vec_copysignf128
	operand order.
	* src/pveclib/vec_f32_ppc.h (vec_copysignf32): Change to match
	operand order from Vector Intrinsic Reference.
	* src/pveclib/vec_f64_ppc.h (vec_copysignf64): Change to match
	operand order from Vector Intrinsic Reference.

	* src/testsuite/arith128_test_f128.c (test_copysignf128):
	Define new __float128 const f128_nsnan.
	Reverse test operands and results to match corrected
	vec_copysignf128() implementation. Remove duplicated tests.
	* src/testsuite/arith128_test_f32.c (test_float_cpsgn):
	Reverse test operands and results to match corrected
	vec_copysignf32() implementation.
	* src/testsuite/arith128_test_f64.c (test_double_cpsgn):
	Reverse test operands and results to match corrected
	vec_copysignf64() implementation.

	* src/testsuite/vec_f32_dummy.c (test_vec_copysignf32):
	new compile test.
	* src/testsuite/vec_f64_dummy.c (test_vec_copysignf64):
	new compile test.

Signed-off-by: Steven Munroe <munroesj52@gmail.com>
@munroesj52 munroesj52 self-assigned this Jan 25, 2022
@munroesj52
Copy link
Contributor Author

munroesj52 commented Jan 25, 2022

We need to think about cherry picking this change for 1.0.4-4 -> 1.0.4-5.

But this is an API change, so should this be a version bump?

@munroesj52
Copy link
Contributor Author

This has aged long enough. So merging so I can move on.

@munroesj52 munroesj52 merged commit 8e670f4 into open-power-sdk:master Feb 2, 2022
@munroesj52 munroesj52 deleted the cpsgn-fix1b branch July 6, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant