| Message ID | 1485275317.6275.98.camel@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 24, 2017 at 08:28:37AM -0800, Carl E. Love wrote: > The following patch fixes an issue with the entries in the table of > built-in functions. All of the entries for a given built-in, must occur > in the table as a single block of entries. Otherwise the code that > searches the table for a given built-in definition will stop looking > once it reaches the end of the initial block of definitions for that > built-in function and subsequent definitions for that built-in will > never be checked. This issue currently occurs with the > ALTIVEC_BUILTIN_VEC_PACKS and P8V_BUILTIN_VEC_VGBBD built-ins. The > patch simply moves the existing entries so the definition for a given > built-in are all together in the same block of entries. Do we need a separate testcase to check for this? Or do those specific builtins need better testcases? Or was the bug obvious already? > Note this issue also exists with the GCC-5 and GCC-6 branches. > > The patch has been tested on powerpc64le-unknown-linux-gnu (Power 8 LE) > with no regressions. > > Is the patch OK for trunk? Yes, thanks! > Assuming this patch is OK, would it be acceptable to post a back port of > the patch for GCC 5 and GCC 6 branches after the patch is in mainline as > long as no issues are seen with this version in the mainline code base? Right; let it simmer on mainline for a while, and then it is approved for backport (if it is the same patch; otherwise please post the version of the patch you checked in to the branches). Do send an email noting you backported it to which branches. Segher > 2017-01-23 Carl Love <cel@us.ibm.com> > > * config/rs6000/rs6000-c (altivec_overloaded_builtins): Fix order > of entries for ALTIVEC_BUILTIN_VEC_PACKS and P8V_BUILTIN_VEC_VGBBD.
On Tue, 2017-01-24 at 11:08 -0600, Segher Boessenkool wrote: > On Tue, Jan 24, 2017 at 08:28:37AM -0800, Carl E. Love wrote: > > The following patch fixes an issue with the entries in the table of > > built-in functions. All of the entries for a given built-in, must occur > > in the table as a single block of entries. Otherwise the code that > > searches the table for a given built-in definition will stop looking > > once it reaches the end of the initial block of definitions for that > > built-in function and subsequent definitions for that built-in will > > never be checked. This issue currently occurs with the > > ALTIVEC_BUILTIN_VEC_PACKS and P8V_BUILTIN_VEC_VGBBD built-ins. The > > patch simply moves the existing entries so the definition for a given > > built-in are all together in the same block of entries. > > Do we need a separate testcase to check for this? Or do those specific > builtins need better testcases? Or was the bug obvious already? I have a list of built-ins that need to have support and test cases added. I found the issue with the ALTIVEC_BUILTIN_VEC_PACKS when I tried to add support for the built-ins: vector signed int vec_packs (vector signed long long x, vector signed long long y); vector unsigned int vec_packs (vector unsigned long long x, vector unsigned long long y); which were in my to do list. What I found was the support for vec_packs is all there but I don't find any test cases for these built-ins. At this point, I do plan to add the vec_pack test cases as part of my work to add the support for the other built-ins on my list. I have the patch in my patch series with the others that need adding. Currently holding off on posting patches since we are only supposed to be posting bug fixes at the moment. Once the bug for the ALTIVEC_BUILTIN_VEC_PACKS built-in was found, I wrote a perl script to scan through the entire table looking for the issue with any other built-in functions. The script found the issue with the P8V_BUILTIN_VEC_VGBBD built-in. My list of built-ins to add doesn't include anything for vec_vgbbd. It would be easy for my to add the test cases for the vec_packs() built-ins to this patch if you would like? I just took a look at the vec_vgbbd() built-in. I grep'd for vgbbd and found the followint two testcases in gcc/testsuite/gcc.target/powerpc/p8vector-builtin-4.c: typedef vector signed char vc_sign; typedef vector unsigned char vc_uns; vc_sign vc_gbb_2 (vc_sign a) { return vec_vgbbd (a); } vc_uns vc_gbb_3 (vc_uns a) { return vec_vgbbd (a); } which correspond to the built-in entries in rs6000-c.c which I didn't move { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, RS6000_BTI_V16QI, RS6000_BTI_V16QI, 0, 0 }, { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI, 0, 0 }, I don't see any tests for the two built-in entries in rs6000-c.c which the patch moves, i.e. { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, RS6000_BTI_V16QI, 0, 0, 0 }, { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, RS6000_BTI_unsigned_V16QI, 0, 0, 0 }, I tried a quick test of adding the following to the test file p8vector-builtin-4.c for these entries: vc_sign vc_gbb_4 (void) { return vec_vgbbd (); } vc_uns vc_gbb_5 (void) { return vec_vgbbd (); } When I compile I get " error: too few arguments to function ‘__builtin_vec_vgbbd’ ". At the moment, I am not sure the point is of a function that returns something but doesn't take any arguments??? I don't see vgbbd listed in the ABI document so not sure if the two built-ins that don't take any arguments are really valid builtins? I would be willing add the above two test cases to the patch if we can figure out how to get them to work. Please let me know if you want me to go ahead with the adding the vec_packs() test cases to the patch or not. Again, not so sure about the vec_vgbbd test cases. Carl Love
On Tue, Jan 24, 2017 at 10:09:23AM -0800, Carl E. Love wrote: > > Do we need a separate testcase to check for this? Or do those specific > > builtins need better testcases? Or was the bug obvious already? [ ... ] > Once the bug for the ALTIVEC_BUILTIN_VEC_PACKS built-in was found, I > wrote a perl script to scan through the entire table looking for the > issue with any other built-in functions. I think we should have the compiler itself do this check, with flag_checking enabled or similar. It is too easy to get this wrong it seems :-( > Please let me know if you want me to go ahead with the adding the vec_packs() test cases > to the patch or not. Again, not so sure about the vec_vgbbd test cases. If you have tests ready, then sure, add them please (in a separate patch is fine). Segher
On Tue, 2017-01-24 at 10:09 -0800, Carl E. Love wrote: > On Tue, 2017-01-24 at 11:08 -0600, Segher Boessenkool wrote: > > On Tue, Jan 24, 2017 at 08:28:37AM -0800, Carl E. Love wrote: > > > The following patch fixes an issue with the entries in the table of > > > built-in functions. All of the entries for a given built-in, must occur > > > in the table as a single block of entries. Otherwise the code that > > > searches the table for a given built-in definition will stop looking > > > once it reaches the end of the initial block of definitions for that > > > built-in function and subsequent definitions for that built-in will > > > never be checked. This issue currently occurs with the > > > ALTIVEC_BUILTIN_VEC_PACKS and P8V_BUILTIN_VEC_VGBBD built-ins. The > > > patch simply moves the existing entries so the definition for a given > > > built-in are all together in the same block of entries. > > > > Do we need a separate testcase to check for this? Or do those specific > > builtins need better testcases? Or was the bug obvious already? > > I have a list of built-ins that need to have support and test cases > added. I found the issue with the ALTIVEC_BUILTIN_VEC_PACKS when I > tried to add support for the built-ins: > > vector signed int vec_packs (vector signed long long x, vector signed long long y); > vector unsigned int vec_packs (vector unsigned long long x, vector unsigned long long y); > > which were in my to do list. What I found was the support for vec_packs > is all there but I don't find any test cases for these built-ins. At > this point, I do plan to add the vec_pack test cases as part of my work > to add the support for the other built-ins on my list. I have the patch > in my patch series with the others that need adding. Currently holding > off on posting patches since we are only supposed to be posting bug > fixes at the moment. > > Once the bug for the ALTIVEC_BUILTIN_VEC_PACKS built-in was found, I > wrote a perl script to scan through the entire table looking for the > issue with any other built-in functions. The script found the issue > with the P8V_BUILTIN_VEC_VGBBD built-in. My list of built-ins to add > doesn't include anything for vec_vgbbd. > > It would be easy for my to add the test cases for the vec_packs() > built-ins to this patch if you would like? > > I just took a look at the vec_vgbbd() built-in. I grep'd for vgbbd and > found the followint two testcases in > gcc/testsuite/gcc.target/powerpc/p8vector-builtin-4.c: > > typedef vector signed char vc_sign; > typedef vector unsigned char vc_uns; > > vc_sign vc_gbb_2 (vc_sign a) > { > return vec_vgbbd (a); > } > > vc_uns vc_gbb_3 (vc_uns a) > { > return vec_vgbbd (a); > } > > which correspond to the built-in entries in rs6000-c.c which I didn't move > > { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, > RS6000_BTI_V16QI, RS6000_BTI_V16QI, 0, 0 }, > { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, > RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI, 0, 0 }, > > I don't see any tests for the two built-in entries in rs6000-c.c which the patch moves, i.e. > > { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, > RS6000_BTI_V16QI, 0, 0, 0 }, > { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, > RS6000_BTI_unsigned_V16QI, 0, 0, 0 }, > > I tried a quick test of adding the following to the test file p8vector-builtin-4.c for these entries: > > vc_sign vc_gbb_4 (void) > { > return vec_vgbbd (); > } > > vc_uns vc_gbb_5 (void) > { > return vec_vgbbd (); > } > > When I compile I get " error: too few arguments to function ‘__builtin_vec_vgbbd’ ". > At the moment, I am not sure the point is of a function that returns > something but doesn't take any arguments??? I don't see vgbbd listed in the ABI document > so not sure if the two built-ins that don't take any arguments are really valid builtins? > I would be willing add the above two test cases to the patch if we can figure out how to > get them to work. Those two entries look bogus to me, and they should just be removed, not moved. I have no idea where they came from. I suspect they were place-holders at one time that snuck into the code by accident. The relevant API interface listed in the ELFv2 ABI is vec_gb, which should support only one interface: vector unsigned char vec_gb (vector unsigned char); So please remove the two bogus interfaces, and make sure we have support for the vec_gb interface in your GCC 8 patch list. Thanks! Bill > > Please let me know if you want me to go ahead with the adding the vec_packs() test cases > to the patch or not. Again, not so sure about the vec_vgbbd test cases. > > Carl Love > > >
Bill: > > > > I don't see any tests for the two built-in entries in rs6000-c.c which the patch moves, i.e. > > > > { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, > > RS6000_BTI_V16QI, 0, 0, 0 }, > > { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, > > RS6000_BTI_unsigned_V16QI, 0, 0, 0 }, > > > > Those two entries look bogus to me, and they should just be removed, not > moved. I have no idea where they came from. I suspect they were > place-holders at one time that snuck into the code by accident. > > The relevant API interface listed in the ELFv2 ABI is vec_gb, which > should support only one interface: > > vector unsigned char vec_gb (vector unsigned char); > > So please remove the two bogus interfaces, and make sure we have support > for the vec_gb interface in your GCC 8 patch list. Thanks! Taking this off list. Bill sorry I missed your email this morning before I committed the patch that moved the vec_vgbbd. I agree the two vec_gbbd entries look bogus to me. There is a test in gcc/testsuite/gcc.target/powerpc/p8vector-builtin-8.c for the vec_gb() interface you mentioned from the ABI that covers this case. I will create and test a patch to remove the bogus entries. I will then roll it into a single patch that fixes the vex_packs entries and adds the missing vex_packs tests. I will then back port the single patch to GCC-5 and GCC-6. I will post the back ported patches to the list in a week or so assuming no issues arise with the changes to mainline. Does that all sound reasonable? Carl Love
diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c index 8b87a0a..92e9849 100644 --- a/gcc/config/rs6000/rs6000-c.c +++ b/gcc/config/rs6000/rs6000-c.c @@ -2154,14 +2154,14 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { RS6000_BTI_unsigned_V8HI, RS6000_BTI_unsigned_V4SI, RS6000_BTI_unsigned_V4SI, 0 }, { ALTIVEC_BUILTIN_VEC_PACKS, ALTIVEC_BUILTIN_VPKSWSS, RS6000_BTI_V8HI, RS6000_BTI_V4SI, RS6000_BTI_V4SI, 0 }, - { ALTIVEC_BUILTIN_VEC_VPKSWSS, ALTIVEC_BUILTIN_VPKSWSS, - RS6000_BTI_V8HI, RS6000_BTI_V4SI, RS6000_BTI_V4SI, 0 }, - { ALTIVEC_BUILTIN_VEC_VPKUWUS, ALTIVEC_BUILTIN_VPKUWUS, - RS6000_BTI_unsigned_V8HI, RS6000_BTI_unsigned_V4SI, RS6000_BTI_unsigned_V4SI, 0 }, { ALTIVEC_BUILTIN_VEC_PACKS, P8V_BUILTIN_VPKUDUS, RS6000_BTI_unsigned_V4SI, RS6000_BTI_unsigned_V2DI, RS6000_BTI_unsigned_V2DI, 0 }, { ALTIVEC_BUILTIN_VEC_PACKS, P8V_BUILTIN_VPKSDSS, RS6000_BTI_V4SI, RS6000_BTI_V2DI, RS6000_BTI_V2DI, 0 }, + { ALTIVEC_BUILTIN_VEC_VPKSWSS, ALTIVEC_BUILTIN_VPKSWSS, + RS6000_BTI_V8HI, RS6000_BTI_V4SI, RS6000_BTI_V4SI, 0 }, + { ALTIVEC_BUILTIN_VEC_VPKUWUS, ALTIVEC_BUILTIN_VPKUWUS, + RS6000_BTI_unsigned_V8HI, RS6000_BTI_unsigned_V4SI, RS6000_BTI_unsigned_V4SI, 0 }, { ALTIVEC_BUILTIN_VEC_VPKSHSS, ALTIVEC_BUILTIN_VPKSHSS, RS6000_BTI_V16QI, RS6000_BTI_V8HI, RS6000_BTI_V8HI, 0 }, { ALTIVEC_BUILTIN_VEC_VPKUHUS, ALTIVEC_BUILTIN_VPKUHUS, @@ -4777,6 +4777,10 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { RS6000_BTI_V16QI, RS6000_BTI_V16QI, 0, 0 }, { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI, 0, 0 }, + { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, + RS6000_BTI_V16QI, 0, 0, 0 }, + { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, + RS6000_BTI_unsigned_V16QI, 0, 0, 0 }, { P9V_BUILTIN_VEC_VINSERT4B, P9V_BUILTIN_VINSERT4B, RS6000_BTI_V16QI, RS6000_BTI_V4SI, @@ -5038,11 +5042,6 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { { P8V_BUILTIN_VEC_VUPKLSW, P8V_BUILTIN_VUPKLSW, RS6000_BTI_bool_V2DI, RS6000_BTI_bool_V4SI, 0, 0 }, - { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, - RS6000_BTI_V16QI, 0, 0, 0 }, - { P8V_BUILTIN_VEC_VGBBD, P8V_BUILTIN_VGBBD, - RS6000_BTI_unsigned_V16QI, 0, 0, 0 }, - { P9V_BUILTIN_VEC_VSLV, P9V_BUILTIN_VSLV, RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI, RS6000_BTI_unsigned_V16QI, 0 },
GCC maintainers: The following patch fixes an issue with the entries in the table of built-in functions. All of the entries for a given built-in, must occur in the table as a single block of entries. Otherwise the code that searches the table for a given built-in definition will stop looking once it reaches the end of the initial block of definitions for that built-in function and subsequent definitions for that built-in will never be checked. This issue currently occurs with the ALTIVEC_BUILTIN_VEC_PACKS and P8V_BUILTIN_VEC_VGBBD built-ins. The patch simply moves the existing entries so the definition for a given built-in are all together in the same block of entries. Note this issue also exists with the GCC-5 and GCC-6 branches. The patch has been tested on powerpc64le-unknown-linux-gnu (Power 8 LE) with no regressions. Is the patch OK for trunk? Assuming this patch is OK, would it be acceptable to post a back port of the patch for GCC 5 and GCC 6 branches after the patch is in mainline as long as no issues are seen with this version in the mainline code base? Carl Love ---------------------------------------------------- gcc/ChangeLog: 2017-01-23 Carl Love <cel@us.ibm.com> * config/rs6000/rs6000-c (altivec_overloaded_builtins): Fix order of entries for ALTIVEC_BUILTIN_VEC_PACKS and P8V_BUILTIN_VEC_VGBBD. --- gcc/config/rs6000/rs6000-c.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)