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