Re: [PATCH, rs6000] PR84220 fix altivec_vec_sld and vec_sldw intrinsic definitions

[Will Schmidt <will_schmidt at vnet dot ibm dot com>]

On Thu, 2018-02-08 at 17:48 -0600, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Feb 07, 2018 at 09:14:59AM -0600, Will Schmidt wrote:
> >   Our VEC_SLD definitions were mistakenly allowing the third argument to be
> > of an invalid type, triggering an ICE (on invalid code) later in the build
> > process.  This fixes those definitions.  The nearby VEC_SLDW definitions have
> > the same issue, those have been fixed as part of this patch too.
> > Testcases have been added to ensure we generate the 'invalid intrinsic'
> > message as is appropriate, instead of ICEing.
> > Giving proper credit, this was found by Peter Bergner while working a
> > different issue. :-)
> > 
> > Sniff-tests passed on P8.  Doing larger reg-test across power systems now.
> > OK for trunk?
> > And,.. do we want this one backported too?
> 
> > diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c
> > index a68be51..26f9990 100644
> > --- a/gcc/config/rs6000/rs6000-c.c
> > +++ b/gcc/config/rs6000/rs6000-c.c
> > @@ -3654,39 +3654,39 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = {
> >    { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_16QI,
> >      RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI },
> >    { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_16QI,
> >      RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI, RS6000_BTI_bool_V16QI, RS6000_BTI_unsigned_V16QI },
> >    { ALTIVEC_BUILTIN_VEC_SLD, ALTIVEC_BUILTIN_VSLDOI_4SF,
> > -    RS6000_BTI_V4SF, RS6000_BTI_V4SF, RS6000_BTI_V4SF, RS6000_BTI_NOT_OPAQUE },
> > +    RS6000_BTI_V4SF, RS6000_BTI_V4SF, RS6000_BTI_V4SF, RS6000_BTI_INTSI },
> 
> It isn't clear to me what RS6000_BTI_NOT_OPAQUE means...  rs6000-c.c says:
> 
>         /* For arguments after the last, we have RS6000_BTI_NOT_OPAQUE in
>            the opX fields.  */
> 
> (whatever that means!), and the following code seems to allow anything in
> such args?  If you understand it, please update some comments somewhere?

I dug in a bit more to try to understand the history and context.

The RS6000_BTI_NOT_OPAQUE entry was added as part of the (large) AltiVec
rewrite (By Paolo Bonzini) in Apr 2005.
The ALTIVEC_BUILTIN_VEC_SLD entries, and the "for arguments after the
last" code chunk in altivec_resolve_overloaded_builtin() was part of
that addition, and pretty much un-touched since that time.

> >    { VSX_BUILTIN_VEC_XXPERMDI, VSX_BUILTIN_XXPERMDI_2DF,
> 
> XXPERMDI is the only other builtin that uses NOT_OPAQUE, does that suffer
> from the same problem?  If so, you can completely delete NOT_OPAQUE it
> seems?

Yes and no.   I've generated a few more tests that show the problem also
included vec_xxperms.   
SO,.. I've updated the patch to fix those references too.

With that change, all references to NOT_OPAQUE in the builtins table are
removed.

(I'll be posting that momentarily while regtests run overnight..)

So then with the idea of cleaning up all remaining references to
_NOT_OPAQUE..  I got stuck.  :-)

The _NOT_OPAQUE definition is the first entry in the (rs6000.h: enum
rs6000_builtin_type_index)

enum rs6000_builtin_type_index
{
-  RS6000_BTI_NOT_OPAQUE,
+  RS6000_BTI_unset,
   RS6000_BTI_opaque_V2SI,


And the only other reference is in this chunk of code in rs6000-c.c:
altivec_resolve_overloaded_builtin() 

      /* For arguments after the last, we have RS6000_BTI_NOT_OPAQUE in
           the opX fields.  */
        for (; desc->code == fcode; desc++)
          {
           if ((desc->op1 == RS6000_BTI_NOT_OPAQUE
                || rs6000_builtin_type_compatible (types[0], desc->op1))
               && (desc->op2 == RS6000_BTI_NOT_OPAQUE
                   || rs6000_builtin_type_compatible (types[1], desc->op2))
               && (desc->op3 == RS6000_BTI_NOT_OPAQUE
                   || rs6000_builtin_type_compatible (types[2], desc->op3)))


So there should no longer be any matches to ...NOT_OPAQUE, but if I
comment out the snippets "== ..NOT_OPAQUE || ", lots of ICE's show up.

which makes me wonder if the check here is more of a "if desc->op1 was
not explicitly set,... " thing.  But it's not clear to me. 

So I'm deliberately not touching this chunk of code at this time. :-)

Thanks
-Will


> 
> So what is/was it for, that is what I wonder.
> 
> Your patch looks fine if you can clear that up :-)
> 
> 
> Segher
>