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.
Thanks for the updated patch. I had said "OK with those fixed" on the older version and it looks like you have indeed fixed the issues I noticed, so this updated patch is OK for trunk. Sorry for not being clarifying earlier Dave > > 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 > > >
