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] Fix for entries in table of overloaded built-in functions


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
> 
> 
> 



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