Re: [PATCH] libgccjit: Add vector permutation and vector access operations

[Antoni Boucher <bouanto@zoho.com>]

David: Ping.

On Thu, 2023-11-30 at 17:16 -0500, Antoni Boucher wrote:
> All of these are fixed in this new patch.
> Thanks for the review.
> 
> On Mon, 2023-11-20 at 18:05 -0500, David Malcolm wrote:
> > On Fri, 2023-11-17 at 17:36 -0500, Antoni Boucher wrote:
> > > Hi.
> > > This patch adds a vector permutation and vector access operations
> > > (bug
> > > 112602).
> > > 
> > > This was split from this patch:
> > > https://gcc.gnu.org/pipermail/jit/2023q1/001606.html
> > > 
> > 
> > Thanks for the patch.
> > 
> > Overall, looks good, but 3 minor nitpicks:
> > 
> > [...snip...]
> > 
> > > diff --git a/gcc/jit/docs/topics/compatibility.rst
> > > b/gcc/jit/docs/topics/compatibility.rst
> > > index ebede440ee4..a764e3968d1 100644
> > > --- a/gcc/jit/docs/topics/compatibility.rst
> > > +++ b/gcc/jit/docs/topics/compatibility.rst
> > > @@ -378,3 +378,13 @@ alignment of a variable:
> > >  --------------------
> > >  ``LIBGCCJIT_ABI_25`` covers the addition of
> > >  :func:`gcc_jit_type_get_restrict`
> > > +
> > > +
> > > +.. _LIBGCCJIT_ABI_26:
> > > +
> > > +``LIBGCCJIT_ABI_26``
> > > +--------------------
> > > +``LIBGCCJIT_ABI_26`` covers the addition of functions to
> > > manipulate vectors:
> > > +
> > > +  * :func:`gcc_jit_context_new_rvalue_vector_perm`
> > > +  * :func:`gcc_jit_context_new_vector_access`
> > > diff --git a/gcc/jit/docs/topics/expressions.rst
> > > b/gcc/jit/docs/topics/expressions.rst
> > > index 42cfee36302..4a45aa13f5c 100644
> > > --- a/gcc/jit/docs/topics/expressions.rst
> > > +++ b/gcc/jit/docs/topics/expressions.rst
> > > @@ -295,6 +295,35 @@ Vector expressions
> > >  
> > >        #ifdef
> > > LIBGCCJIT_HAVE_gcc_jit_context_new_rvalue_from_vector
> > >  
> > > +.. function:: gcc_jit_rvalue * \
> > > +              gcc_jit_context_new_rvalue_vector_perm
> > > (gcc_jit_context *ctxt, \
> > > +                                                     
> > > gcc_jit_location *loc, \
> > > +                                                     
> > > gcc_jit_rvalue *elements1, \
> > > +                                                     
> > > gcc_jit_rvalue *elements2, \
> > > +                                                     
> > > gcc_jit_rvalue *mask);
> > > +
> > > +   Build a permutation of two vectors.
> > > +
> > > +   "elements1" and "elements2" should have the same type.
> > > +   The length of "mask" and "elements1" should be the same.
> > > +   The element type of "mask" should be integral.
> > > +   The size of the element type of "mask" and "elements1" should
> > > be the same.
> > > +
> > > +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_25`; you can
> > > test for
> >                                                        ^^
> > Should be 26
> > 
> > [...snip...]
> > 
> > >  Unary Operations
> > >  ****************
> > >  
> > > @@ -1020,3 +1049,27 @@ Field access is provided separately for
> > > both
> > > lvalues and rvalues.
> > >        PTR[INDEX]
> > >  
> > >     in C (or, indeed, to ``PTR + INDEX``).
> > > +
> > > +.. function:: gcc_jit_lvalue *\
> > > +              gcc_jit_context_new_vector_access (gcc_jit_context
> > > *ctxt,\
> > > +                                                
> > > gcc_jit_location
> > > *loc,\
> > > +                                                 gcc_jit_rvalue
> > > *vector,\
> > > +                                                 gcc_jit_rvalue
> > > *index)
> > > +
> > > +   Given an rvalue of vector type ``T __attribute__
> > > ((__vector_size__ (SIZE)))``, get the element `T` at
> > > +   the given index.
> > > +
> > > +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_25`; you can
> > > test for
> >                                                        ^^
> > 
> > Likewise here.
> > 
> > [...snip...]
> > 
> > > @@ -4071,6 +4107,79 @@ gcc_jit_context_new_rvalue_from_vector
> > > (gcc_jit_context *ctxt,
> > >       (gcc::jit::recording::rvalue **)elements);
> > >  }
> > >  
> > > +/* Public entrypoint.  See description in libgccjit.h.
> > > +
> > > +   After error-checking, the real work is done by the
> > > +   gcc::jit::recording::context::new_rvalue_vector_perm method,
> > > in
> > > +   jit-recording.cc.  */
> > > +
> > > +gcc_jit_rvalue *
> > > +gcc_jit_context_new_rvalue_vector_perm (gcc_jit_context *ctxt,
> > > +					gcc_jit_location *loc,
> > > +					gcc_jit_rvalue
> > > *elements1,
> > > +					gcc_jit_rvalue
> > > *elements2,
> > > +					gcc_jit_rvalue *mask)
> > > +{
> > > +  RETURN_NULL_IF_FAIL (ctxt, NULL, loc, "NULL ctxt");
> > > +  JIT_LOG_FUNC (ctxt->get_logger ());
> > > +
> > > +  /* LOC can be NULL.  */
> > 
> > ...but "elements1", "elements2", and "mask" must not be NULL, as
> > they're dereferenced below.  So this is going to need something
> > like
> > the following (untested):
> > 
> >   RETURN_NULL_IF_FAIL (elements1, ctxt, loc, "NULL elements1");
> >   RETURN_NULL_IF_FAIL (elements2, ctxt, loc, "NULL elements2");
> >   RETURN_NULL_IF_FAIL (mask, ctxt, loc, "NULL mask");
> > 
> > [...snip...]
> > 
> > OK with those fixed.
> > 
> > Thanks
> > Dave
> > 
>