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

[Bill Schmidt <wschmidt@linux.vnet.ibm.com>]

> On Feb 13, 2018, at 5:28 PM, Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
> 
> 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?

The intent is that each entry in the table has a maximum of one result type
and three argument types.  If a built-in only takes one argument, then the second
and third argument entries in the table should be flagged as
RS6000_BTI_NOT_OPAQUE.  I have never understood why that particular
(misleading) term was used.  But basically it means (as Will found in the code
chunk below) that you shouldn't check type compatibility because we don't
expect an argument in that position.

At least that's always been my understanding.  I could be wrong.  It seems
very odd that you are finding it easy to remove this (only a few entries), so
I might be way off base.

Bill

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