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

[David Malcolm <dmalcolm@redhat.com>]

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