| Message ID | e57a8f3f-e356-7153-cfdf-80d179a0b651@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] rs6000: correct vector sign extend built-ins on Big Endian [PR108812] | expand |
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
On Mon, Mar 27, 2023 at 03:14:26PM +0800, Kewen.Lin wrote: > on 2023/3/27 14:16, HAO CHEN GUI wrote: > > 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. When extending from sizes A to B the rightmost A in every B. That is the same in every endianness, yes -- it is what the machine insns do after all, it has nothing to do with how the elements are numbered in the ABI :-) > 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). A very welcome cleanup :-) > 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? Yes please :-) Segher
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; }) @@ -4968,14 +4961,7 @@ (define_expand "vsignextend_hi_<mode>" UNSPEC_VSX_SIGN_EXTEND))] "TARGET_P9_VECTOR" { - if (BYTES_BIG_ENDIAN) - { - rtx tmp = gen_reg_rtx (V8HImode); - emit_insn (gen_altivec_vrevev8hi2(tmp, operands[1])); - emit_insn (gen_vsx_sign_extend_hi_<mode>(operands[0], tmp)); - } - else - emit_insn (gen_vsx_sign_extend_hi_<mode>(operands[0], operands[1])); + emit_insn (gen_vsx_sign_extend_hi_<mode>(operands[0], operands[1])); DONE; }) @@ -4987,24 +4973,6 @@ (define_insn "vsx_sign_extend_si_v2di" "vextsw2d %0,%1" [(set_attr "type" "vecexts")]) -(define_expand "vsignextend_si_v2di" - [(set (match_operand:V2DI 0 "vsx_register_operand" "=v") - (unspec:V2DI [(match_operand:V4SI 1 "vsx_register_operand" "v")] - UNSPEC_VSX_SIGN_EXTEND))] - "TARGET_P9_VECTOR" -{ - if (BYTES_BIG_ENDIAN) - { - rtx tmp = gen_reg_rtx (V4SImode); - - emit_insn (gen_altivec_vrevev4si2(tmp, operands[1])); - emit_insn (gen_vsx_sign_extend_si_v2di(operands[0], tmp)); - } - else - emit_insn (gen_vsx_sign_extend_si_v2di(operands[0], operands[1])); - DONE; -}) - ;; Sign extend DI to TI. We provide both GPR targets and Altivec targets on ;; power10. On earlier systems, the machine independent code will generate a ;; shift left to sign extend the 64-bit value to 128-bit. diff --git a/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c b/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c index fdcad019b96..03c0f1201e4 100644 --- a/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c +++ b/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c @@ -34,7 +34,12 @@ int main () /* test sign extend byte to word */ vec_arg_qi = (vector signed char) {1, 2, 3, 4, 5, 6, 7, 8, -1, -2, -3, -4, -5, -6, -7, -8}; + +#ifdef __BIG_ENDIAN__ + vec_expected_wi = (vector signed int) {4, 8, -4, -8}; +#else vec_expected_wi = (vector signed int) {1, 5, -1, -5}; +#endif vec_result_wi = vec_signexti (vec_arg_qi); @@ -54,7 +59,12 @@ int main () /* test sign extend byte to double */ vec_arg_qi = (vector signed char){1, 2, 3, 4, 5, 6, 7, 8, -1, -2, -3, -4, -5, -6, -7, -8}; + +#ifdef __BIG_ENDIAN__ + vec_expected_di = (vector signed long long int){8, -8}; +#else vec_expected_di = (vector signed long long int){1, -1}; +#endif vec_result_di = vec_signextll(vec_arg_qi); @@ -72,7 +82,12 @@ int main () /* test sign extend short to word */ vec_arg_hi = (vector signed short int){1, 2, 3, 4, -1, -2, -3, -4}; + +#ifdef __BIG_ENDIAN__ + vec_expected_wi = (vector signed int){2, 4, -2, -4}; +#else vec_expected_wi = (vector signed int){1, 3, -1, -3}; +#endif vec_result_wi = vec_signexti(vec_arg_hi); @@ -90,7 +105,12 @@ int main () /* test sign extend short to double word */ vec_arg_hi = (vector signed short int ){1, 3, 5, 7, -1, -3, -5, -7}; + +#ifdef __BIG_ENDIAN__ + vec_expected_di = (vector signed long long int){7, -7}; +#else vec_expected_di = (vector signed long long int){1, -1}; +#endif vec_result_di = vec_signextll(vec_arg_hi); @@ -108,7 +128,12 @@ int main () /* test sign extend word to double word */ vec_arg_wi = (vector signed int ){1, 3, -1, -3}; + +#ifdef __BIG_ENDIAN__ + vec_expected_di = (vector signed long long int){3, -3}; +#else vec_expected_di = (vector signed long long int){1, -1}; +#endif vec_result_di = vec_signextll(vec_arg_wi);