[PATCH 03/34] rs6000: Add the rest of the [altivec] stanza to the builtins file

Segher Boessenkool segher@kernel.crashing.org
Sun Aug 8 20:27:10 GMT 2021


Hi!

On Sun, Aug 08, 2021 at 11:53:38AM -0500, Bill Schmidt wrote:
> On 8/6/21 7:01 PM, Segher Boessenkool wrote:
> >On Thu, Jul 29, 2021 at 08:30:50AM -0500, Bill Schmidt wrote:
> >>+  const vsc __builtin_altivec_abss_v16qi (vsc);
> >>+    ABSS_V16QI altivec_abss_v16qi {}
> >>+
> >>+  const vsi __builtin_altivec_abss_v4si (vsi);
> >>+    ABSS_V4SI altivec_abss_v4si {}
> >>+
> >>+  const vss __builtin_altivec_abss_v8hi (vss);
> >>+    ABSS_V8HI altivec_abss_v8hi {}
> >Is there any ordering used here?  What is it, then?  Just alphabetical?
> >
> >That order does not really allow breaking things up into groups, which
> >is the main tool to keep things manageable.
> 
> Yes, within each stanza, the ordering is alphabetical by built-in name.  
> It seems to me that any other ordering is arbitrary and prone to 
> requiring exceptions, so in the end you just end up with a mess where 
> nobody knows where to put the next builtin added. That's certainly what 
> happened with the old support.

Yeah, there is no great answer here :-(  You have thought about it in
any case, so let's see where this goes.

> >>+  const vsc __builtin_vec_init_v16qi (signed char, signed char, signed 
> >>char, signed char, signed char, signed char, signed char, signed char, 
> >>signed char, signed char, signed char, signed char, signed char, signed 
> >>char, signed char, signed char);
> >That is a very long line, can you do something about that, or is that
> >forced by the file format?  Can you use just "char"?  "signed char" is a
> >very strange choice.
> 
> Right now, long lines are there because the parser doesn't support 
> breaking up the line.  I have an additional patch I put together 
> recently that allows the use of escape-newline to break up these lines.  
> I am planning to submit that once we get through the current patch set.

Okido.  What about the signed char though?

> >>+  pcvoid_type_node
> >>+    = build_pointer_type (build_qualified_type (void_type_node,
> >>+						TYPE_QUAL_CONST));
> >A const void?  Interesting.  You are building a pointer to a const void
> >here, not a const pointer to void.  Is that what you wanted?
> >
> >(And yes I do realise this is just moved, not new code).
> 
> 
> Sorry, I misdocumented this below.  I'll review and make sure this is 
> correct everywhere.

"const void" is meaningless, and maybe even invalid C.  I think the code
is wrong, not (just) the documentation!  This wants to be
  void *const
but it is
  const void *
as far as I can see?

As I said, this isn't new code, but it seems very wrong!


Segher


More information about the Gcc-patches mailing list