Re: [PATCH v3 02/16] target/arm: Create gen_gvec_{u,s}{rshr,rsra}

[Peter Maydell]

On Tue, 12 May 2020 at 14:09, Peter Maydell <address@hidden> wrote:
>
> On Fri, 8 May 2020 at 16:22, Richard Henderson
> <address@hidden> wrote:
> >
> > Create vectorized versions of handle_shri_with_rndacc
> > for shift+round and shift+round+accumulate.  Add out-of-line
> > helpers in preparation for longer vector lengths from SVE.
> >
> > Signed-off-by: Richard Henderson <address@hidden>
> > +    /* tszimm encoding produces immediates in the range [1..esize] */
> > +    tcg_debug_assert(shift > 0);
>
> This assert can be triggered:

(well, not this assert, but the equivalent one in gen_gvec_ursra)


> > +static void gen_srshr_vec(unsigned vece, TCGv_vec d, TCGv_vec a, int64_t 
> > sh)
> > +{
> > +    TCGv_vec t = tcg_temp_new_vec_matching(d);
> > +    TCGv_vec ones = tcg_temp_new_vec_matching(d);
> > +
> > +    tcg_gen_shri_vec(vece, t, a, sh - 1);
> > +    tcg_gen_dupi_vec(vece, ones, 1);
> > +    tcg_gen_and_vec(vece, t, t, ones);
> > +    tcg_gen_sari_vec(vece, d, a, sh);
> > +    tcg_gen_add_vec(vece, d, d, t);
> > +
> > +    tcg_temp_free_vec(t);
> > +    tcg_temp_free_vec(ones);
> > +}

+void gen_gvec_srshr(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs,
+                    int64_t shift, uint32_t opr_sz, uint32_t max_sz)
+{
+    static const TCGOpcode vecop_list[] = {
+        INDEX_op_shri_vec, INDEX_op_sari_vec, INDEX_op_add_vec, 0
+    };

Is there documentation somewhere of which vector operations don't
need to be listed in the vecop list? Here gen_srshr_vec() also
uses 'dupi_vec' and 'and_vec', which aren't listed, presumably
because we guarantee them to be implemented? (Hopefully we don't
encounter a future host vector architecture which breaks that
assumption :-))

> > @@ -5269,6 +5685,28 @@ static int disas_neon_data_insn(DisasContext *s, 
> > uint32_t insn)
> >                      }
> >                      return 0;
> >
> > +                case 2: /* VRSHR */
> > +                    /* Right shift comes here negative.  */
> > +                    shift = -shift;
> > +                    if (u) {
> > +                        gen_gvec_urshr(size, rd_ofs, rm_ofs, shift,
> > +                                       vec_size, vec_size);
> > +                    } else {
> > +                        gen_gvec_srshr(size, rd_ofs, rm_ofs, shift,
> > +                                       vec_size, vec_size);
> > +                    }
> > +                    return 0;
> > +
> > +                case 3: /* VRSRA */
> > +                    if (u) {
> > +                        gen_gvec_ursra(size, rd_ofs, rm_ofs, shift,
> > +                                       vec_size, vec_size);
> > +                    } else {
> > +                        gen_gvec_srsra(size, rd_ofs, rm_ofs, shift,
> > +                                       vec_size, vec_size);
> > +                    }
> > +                    return 0;
>
> I think the VRSRA case needs the same "shift = -shift" as VRSHR.

With this bug fixed,
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM