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
>
