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

Compiler bug in vec_cpsgn reverses operands. #158

Open
munroesj52 opened this issue Dec 6, 2021 · 2 comments · Fixed by #170
Open

Compiler bug in vec_cpsgn reverses operands. #158

munroesj52 opened this issue Dec 6, 2021 · 2 comments · Fixed by #170
Assignees

Comments

@munroesj52
Copy link
Contributor

munroesj52 commented Dec 6, 2021

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 the ISA say the sign is copied from operand a into b.

Unfortunately PVECLIB followed the original vec_cpsgn() for the implementation of:
vec_copysignf32(),
vec_copysignf64(),
and
vec_copysignf128()

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.

  • GCC will apply the fix starting with GCC-9 (AT13.0)
  • Clang has already applied the fix starting with Clang-12

PVECLIBs vec_copysignf32() and vec_copysignf64() used vec_cpsgn() as the implementation for __ARCH_PWR7 and above. So applications may see incompatible changes depending on the compiler used. vec_copysignf128() will retain the original implementation and operand order.

So obviously PVECLIB should insure that applications get consistent results across compiler versions. But should PVECLIB retain the existing (self consistent and documented) operand order. Or change to match intended order defined in the Intrinsic reference and the ISA?

@munroesj52 munroesj52 self-assigned this Dec 6, 2021
@ThinkOpenly
Copy link
Contributor

I vote for being consistent with the intended / fixed behavior of the ISA and Intrinsics reference. The barrier to entry/exit from using pveclib should be low, so matching future / long-term behavior is preferred.

@munroesj52
Copy link
Contributor Author

This is a configuration nightmare. The compile changes to fix vec_cpsgn will be applied to GCC 9 and later (clang12 and later), but not earlier versions. The fixes will take awhile to get accepted by GCC/Clang community, then the distro's, who will schedule and distribute updates, and finally applied on the users system. Clocked in random months.

So we need a fail-safe solution that will provide the expected/correct result for:

  • Older compilers that will never get the update.
  • Currently supported compilers before the update is distributed/installed.
  • Currently supported compilers after the update is distributed/installed.

I am thinking something like this:
#if _ARCH_PWR7
#ifdef PVECLIB_CPSGN_FIXED
return (vec_cpsgn (vf32x, vf32y));
#else
vf32_t result;
asm(
"xvcpsgnsp %x0,%x1,%x2;\n"
: "=wa" (result)
: "wa" (vf32x), "wa" (vf32y)
:);
return (result);
#endif
#else
...
#endif

This assumes that any compiler that defines _ARCH_PWR7 will assemble xvcpsgnsp/xvcpsgndp. So if PVECLIB_CPSGN_FIXED is not defined we will generate reasonable code that is correct but perhaps not as optimal as the vec_cpsgn built-in would generate.

I can add a configure test the will compile and test vec_cpsgn() for correct results and define PVECLIB_CPSGN_FIXED for the PVECLIB build. But unless applications use this (or equivalent) configure test, that application will get the fail-safe code.

The alternative is to add some really complicated cpp logic to vec_common_ppc.h (or perhaps add a vec_config_ppc.h) that defines PVECLIB_CPSGN_FIXED if safe and appropriate. This requires checking multiple compiler releases for specific major and patch levels. Most of which we will not know until I see specific compilers released into the wild and can test them.

munroesj52 added a commit to munroesj52/pveclib that referenced this issue Dec 17, 2021
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 <[email protected]>
munroesj52 added a commit to munroesj52/pveclib that referenced this issue Jan 25, 2022
Resulve merge conflict open-power-sdk#159

	* src/testsuite/arith128_test_f128.c (test_copysignf128):
	result merge conflict.

Signed-off-by: Steven Munroe <[email protected]>
munroesj52 added a commit to munroesj52/pveclib that referenced this issue Jan 25, 2022
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 <[email protected]>
munroesj52 added a commit that referenced this issue Feb 2, 2022
Fix vec_copysign implementations per issue #158, Part 1B.
@munroesj52 munroesj52 linked a pull request Sep 16, 2022 that will close this issue
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 a pull request may close this issue.

2 participants