Hi Haochen,
Thanks for fixing this.
on 2023/3/27 14:16, HAO CHEN GUI wrote:
> Hi,
> This patch removes byte reverse operation before vector integer sign
> extension on Big Endian. These built-ins require to sign extend the rightmost
> element. So both BE and LE should do the same operation and the byte reversion
> is no need. This patch fixes it. Now these built-ins have the same behavior on
> all compilers. The test case is modified also.
Nice, I think this change aligns with what's in the documentation:
"Each element of the result is produced by sign-extending the element of the
input
vector that would fall in the least significant portion of the result element.
For
example, a sign-extension of a vector signed char to a vector signed long long
will
sign extend the rightmost byte of each doubleword."
>
> The patch passed regression test on Power Linux platforms.
>
> Thanks
> Gui Haochen
>
> ChangeLog
> rs6000: correct vector sign extend builtins on Big Endian
>
> gcc/
> PR target/108812
> * config/rs6000/vsx.md (vsignextend_qi_<mode>): Remove byte reverse
> for Big Endian.
> (vsignextend_hi_<mode>): Likewise.
> (vsignextend_si_v2di): Remove.
> * config/rs6000/rs6000-builtins.def (__builtin_altivec_vsignextsw2d):
> Set bif-pattern to vsx_sign_extend_si_v2di.
>
> gcc/testsuite/
> PR target/108812
> * gcc.target/powerpc/p9-sign_extend-runnable.c: Set different expected
> vectors for Big Endian.
>
>
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000-builtins.def
> b/gcc/config/rs6000/rs6000-builtins.def
> index f76f54793d7..059a455b388 100644
> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -2699,7 +2699,7 @@
> VSIGNEXTSH2W vsignextend_hi_v4si {}
>
> const vsll __builtin_altivec_vsignextsw2d (vsi);
> - VSIGNEXTSW2D vsignextend_si_v2di {}
> + VSIGNEXTSW2D vsx_sign_extend_si_v2di {}
>
> const vsc __builtin_altivec_vslv (vsc, vsc);
> VSLV vslv {}
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index 992fbc983be..9e9b33f56ab 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -4941,14 +4941,7 @@ (define_expand "vsignextend_qi_<mode>"
> UNSPEC_VSX_SIGN_EXTEND))]
> "TARGET_P9_VECTOR"
> {
> - if (BYTES_BIG_ENDIAN)
> - {
> - rtx tmp = gen_reg_rtx (V16QImode);
> - emit_insn (gen_altivec_vrevev16qi2(tmp, operands[1]));
> - emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], tmp));
> - }
> - else
> - emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], operands[1]));
> + emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], operands[1]));
> DONE;
> })
I think the whole define_expand can be removed, we can just use the
define_insn names vsx_sign_extend_qi_* in rs6000-builtins.def (just
like what you changed for __builtin_altivec_vsignextsw2d).
This comment is also applied for vsx_sign_extend_hi_*,
vsx_sign_extend_si_* and vsx_sign_extend_v2di_*.
One interesting thing is that we used qi/hi/si in the name for
V16QI/V8HI/V4SI but used v2di for V2DI, could you also adjust the
names from vsx_sign_extend_{qi,hi,si}_* to ..._{v16qi,v8hi,v4si}_*
then make them adopt the same naming style?
BR,
Kewen
