This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH,rs6000] PR79395: Fix compile error with -mcpu=power9 and -mno-vsx and __builtin_vec_cmpne_p


Hi Kelvin,

On Tue, Feb 28, 2017 at 03:46:20PM -0700, Kelvin Nilsen wrote:
> PR 79395 reports a problem that arises when the preprocessor believes
> that the target supports Power9 but the gcc compiler believes that
> Power9 is not supported.
> 
> This patch addresses this inconsistency by introducing a new
> preprocessor macro named __POWER9_VECTOR__ which is automatically
> defined if the current gcc configuration, as adjusted by gcc command
> line options, supports Power9.  Previously, certain macro definitions
> that were supplied in altivec.h were conditioned upon the _ARCH_PWR9
> macro, which represents statically whether the compiler can support
> Power9, but ignores any command-line options that might disable the
> Power9 support in this run of the compiler.  Also addressed in this
> patch is elimination of the xvcmpnesp and xvcmpnedp instructions, which
> are not currently supported.  

I don't like this much at all.  We should not have command line options
to disable random instructions.  In a sane world we could just test for
_ARCH_PWR9 and VSX instead of having this error-prone combinatorial
explosion of macros.

> This patch has been demonstrated to fix the problems identified in the
> test case mentioned in the PR 79395 report.
> 
> This patch has been bootstrapped and tested on
> powerpc64le-unknown-linux with no regressions.
> 
> Is this ok for trunk?

Okay.  Some trivial comments:


> 	PR target/79395
> 	* config/rs6000/altivec.h (vec_ctz and others): Change the
> 	preprocessor macro that controls conditional compilation from
> 	_ARCH_PWR9 to __POWER9_VECTOR.

It is __POWER9_VECTOR__.

> 	(vec_all_ne): Change macro definition to use a power9-specific
> 	expansion under #ifdef __POWER9_VECTOR CONTROL (instead of
> 	_ARCH_PWR9 control).

Same; and you don't want to SHOUT control I think ;-)

> 	* config/rs6000/rs6000-c.c (rs6000_target_modify_macros): Add
> 	support for predefined __POWER9_VECTOR__ macro to indicate that
> 	Power9 instruction selection is enabled.
> 	(altivec_overloaded_builtins): Remove extraneous
> 	ALTIVEC_BUILTIN_VEC_CMPNE entry for overloaded 

Trailing space.

> 	Power9 for impelmentations of vec_cmpne.  Change the signature for

s/impel/imple/

> @@ -521,9 +521,9 @@ __altivec_scalar_pred(vec_all_nez,
>  __altivec_scalar_pred(vec_any_eqz,
>    __builtin_vec_vcmpnez_p (__CR6_LT_REV, a1, a2))
>  __altivec_scalar_pred(vec_all_ne,
> -  __builtin_vec_vcmpne_p (__CR6_LT, a1, a2))
> +		      __builtin_vec_allne_p (a1, a2))
>  __altivec_scalar_pred(vec_any_eq,
> -  __builtin_vec_vcmpne_p (__CR6_LT_REV, a1, a2))
> +		      __builtin_vec_anyeq_p (a1, a2))
>  #endif

Please indent these the same as the surrounding code.

Thanks,


Segher


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]