| Message ID | 073e4ca4-be47-4037-8719-035c4781b1a9@linux.ibm.com |
|---|---|
| Headers | show |
| Series | rs6000, built-in cleanup patch series | expand |
Hi Carl, on 2024/6/14 03:40, Carl Love wrote: > GCC maintainers: > > The patch has been updated per the feedback from version 3. Please let me know it the patch is acceptable for mainline. > > Thanks. > > Carl > > ---------------------------------------------------------------------------------- > > rs6000, remove vector set and vector init built-ins > > The vector init built-ins: > > __builtin_vec_init_v16qi, __builtin_vec_init_v8hi, > __builtin_vec_init_v4si, __builtin_vec_init_v4sf, > __builtin_vec_init_v2di, __builtin_vec_init_v2df, > __builtin_vec_init_v1ti > > perform the same operation as initializing the vector in C code. For > example: > > result_v4si = __builtin_vec_init_v4si (1, 2, 3, 4); > result_v4si = {1, 2, 3, 4}; > > These two constructs were tested and verified they generate identical > assembly instructions with no optimization and -O3 optimization. > > The vector set built-ins: > > __builtin_vec_set_v16qi, __builtin_vec_set_v8hi. > __builtin_vec_set_v4si, __builtin_vec_set_v4sf, > __builtin_vec_set_v1ti, __builtin_vec_set_v2di, > __builtin_vec_set_v2df > > perform the same operation as setting a specific element in the vector in > C code. For example: > > src_v4si = __builtin_vec_set_v4si (src_v4si, int_val, index); > src_v4si[index] = int_val; > > The built-in actually generates more instructions than the inline C code > with no optimization but is identical with -O3 optimizations. > > All of the above built-ins that are removed do not have test cases and > are not documented. > > Built-ins __builtin_vec_set_v1ti __builtin_vec_set_v2di, > __builtin_vec_set_v2df are not removed as they are used in function > resolve_vec_insert() in file rs6000-c.cc. > > The built-ins are removed as they don't provide any benefit over just > using C code. > > gcc/ChangeLog: > * config/rs6000/rs6000-builtins.def (__builtin_vec_init_v16qi, > __builtin_vec_init_v4sf, __builtin_vec_init_v4si, > __builtin_vec_init_v8hi, __builtin_vec_init_v1ti, > __builtin_vec_init_v2df, __builtin_vec_init_v2di, > __builtin_vec_set_v16qi, __builtin_vec_set_v4sf, > __builtin_vec_set_v4si, __builtin_vec_set_v8hi): Remove > built-in definitions. > --- > gcc/config/rs6000/rs6000-builtins.def | 44 +++------------------------ > 1 file changed, 4 insertions(+), 40 deletions(-) > > diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def > index 02aa04e5698..053dc0115d2 100644 > --- a/gcc/config/rs6000/rs6000-builtins.def > +++ b/gcc/config/rs6000/rs6000-builtins.def > @@ -1118,37 +1118,6 @@ > const signed short __builtin_vec_ext_v8hi (vss, signed int); > VEC_EXT_V8HI nothing {extract} > > - const vsc __builtin_vec_init_v16qi (signed char, signed char, signed char, \ > - signed char, signed char, signed char, signed char, signed char, \ > - signed char, signed char, signed char, signed char, signed char, \ > - signed char, signed char, signed char); > - VEC_INIT_V16QI nothing {init} I just realized this {init} is customized for vec_init only, these removed vec_init bifs are the only users of it, so we should remove this attribute as well. Sorry that I should have found and pointed out this in the previous review. I think it means some removals are needed on: 1) comments in rs6000-builtins.def ; init Process as a vec_init function 2) related gen code for this attribute bit, like: fprintf (header_file, "#define bif_init_bit\t\t(0x00000001)\n"); fprintf (header_file, "#define bif_is_init(x)\t\t((x).bifattrs & bif_init_bit)\n"); if (bifp->attrs.isinit) fprintf (init_file, " | bif_init_bit"); The others look good to me! BR, Kewen
Kewen: On 6/18/24 20:04, Kewen.Lin wrote: > Hi Carl, > > on 2024/6/14 03:40, Carl Love wrote: >> GCC maintainers: >> >> The patch has been updated per the feedback from version 3. Please let me know it the patch is acceptable for mainline. >> >> Thanks. >> >> Carl >> >> ---------------------------------------------------------------------------------- >> >> rs6000, remove vector set and vector init built-ins >> >> The vector init built-ins: >> >> __builtin_vec_init_v16qi, __builtin_vec_init_v8hi, >> __builtin_vec_init_v4si, __builtin_vec_init_v4sf, >> __builtin_vec_init_v2di, __builtin_vec_init_v2df, >> __builtin_vec_init_v1ti >> >> perform the same operation as initializing the vector in C code. For >> example: >> >> result_v4si = __builtin_vec_init_v4si (1, 2, 3, 4); >> result_v4si = {1, 2, 3, 4}; >> >> These two constructs were tested and verified they generate identical >> assembly instructions with no optimization and -O3 optimization. >> >> The vector set built-ins: >> >> __builtin_vec_set_v16qi, __builtin_vec_set_v8hi. >> __builtin_vec_set_v4si, __builtin_vec_set_v4sf, >> __builtin_vec_set_v1ti, __builtin_vec_set_v2di, >> __builtin_vec_set_v2df >> >> perform the same operation as setting a specific element in the vector in >> C code. For example: >> >> src_v4si = __builtin_vec_set_v4si (src_v4si, int_val, index); >> src_v4si[index] = int_val; >> >> The built-in actually generates more instructions than the inline C code >> with no optimization but is identical with -O3 optimizations. >> >> All of the above built-ins that are removed do not have test cases and >> are not documented. >> >> Built-ins __builtin_vec_set_v1ti __builtin_vec_set_v2di, >> __builtin_vec_set_v2df are not removed as they are used in function >> resolve_vec_insert() in file rs6000-c.cc. >> >> The built-ins are removed as they don't provide any benefit over just >> using C code. >> >> gcc/ChangeLog: >> * config/rs6000/rs6000-builtins.def (__builtin_vec_init_v16qi, >> __builtin_vec_init_v4sf, __builtin_vec_init_v4si, >> __builtin_vec_init_v8hi, __builtin_vec_init_v1ti, >> __builtin_vec_init_v2df, __builtin_vec_init_v2di, >> __builtin_vec_set_v16qi, __builtin_vec_set_v4sf, >> __builtin_vec_set_v4si, __builtin_vec_set_v8hi): Remove >> built-in definitions. >> --- >> gcc/config/rs6000/rs6000-builtins.def | 44 +++------------------------ >> 1 file changed, 4 insertions(+), 40 deletions(-) >> >> diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def >> index 02aa04e5698..053dc0115d2 100644 >> --- a/gcc/config/rs6000/rs6000-builtins.def >> +++ b/gcc/config/rs6000/rs6000-builtins.def >> @@ -1118,37 +1118,6 @@ >> const signed short __builtin_vec_ext_v8hi (vss, signed int); >> VEC_EXT_V8HI nothing {extract} >> >> - const vsc __builtin_vec_init_v16qi (signed char, signed char, signed char, \ >> - signed char, signed char, signed char, signed char, signed char, \ >> - signed char, signed char, signed char, signed char, signed char, \ >> - signed char, signed char, signed char); >> - VEC_INIT_V16QI nothing {init} > I just realized this {init} is customized for vec_init only, these removed vec_init > bifs are the only users of it, so we should remove this attribute as well. Sorry that > I should have found and pointed out this in the previous review. I think it means > some removals are needed on: > > 1) comments in rs6000-builtins.def > ; init Process as a vec_init function > > 2) related gen code for this attribute bit, like: > > fprintf (header_file, "#define bif_init_bit\t\t(0x00000001)\n"); > fprintf (header_file, > "#define bif_is_init(x)\t\t((x).bifattrs & bif_init_bit)\n"); > if (bifp->attrs.isinit) > fprintf (init_file, " | bif_init_bit"); > OK, Yes, we can remove the attribute string for the vec_init built-in. In addition to the code you mentioned, we will need to remove the uses of bif_init_bit, bif_is_init and the function altivec_expand_vec_init_builtin. Carl
GCC maintainers: I have addressed the comments to the five patches in the series that have not yet been approved. The patches that have already been approved are 1, 3, 5, 6, 8, 9, 10, and 12. The remaining patches all have fairly minor fixes requested. I will just post version 4 of these patches here. The goal is to commit the entire series all at once as they are all related. So I a holding off committing the approved patches. Thank you for your time and feedback of these patches. The entire patch series has been tested on Power 10 LE, Power 9 BE with no regression failures. Carl