This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, rs6000] PR84220 fix altivec_vec_sld and vec_sldw intrinsic definitions
- From: Will Schmidt <will_schmidt at vnet dot ibm dot com>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- Cc: gcc-patches at gcc dot gnu dot org, David Edelsohn <dje dot gcc at gmail dot com>, Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>, Peter Bergner <bergner at vnet dot ibm dot com>, Michael Meissner <meissner at linux dot vnet dot ibm dot com>
- Date: Fri, 09 Feb 2018 10:14:19 -0600
- Subject: Re: [PATCH, rs6000] PR84220 fix altivec_vec_sld and vec_sldw intrinsic definitions
- Authentication-results: sourceware.org; auth=none
- References: <1518016499.11602.261.camel@brimstone.rchland.ibm.com> <20180208234826.GM21977@gate.crashing.org>
- Reply-to: 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 do not... only got as far as figuring out it wasn't right for
vec_sld*().
I'll poke around a bit to see what I can figure out. May need to punt
to (???) to understand the intent. Mike?/Peter?
A bit more context around that usage is:
[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))
...
with that check repeated against types[1], desc->op2,.. etc.
> > { 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?
Not sure.
> So what is/was it for, that is what I wonder.
>
> Your patch looks fine if you can clear that up :-)
Heh, Ok.
>
>
> Segher
>