Hi Philippe,
On 2026-10-24 19:00, Philippe Mathieu-Daudé wrote:
> On 10/10/26 19:46, Aurelien Jarno wrote:
> > For logical vector operations (in the case of the RISC-V backend that
> > corresponds to and_vec, or_vec and xor_vec), VECE is unused. However,
> > tcg_target_const_match() always treat it as an element size to extract
> > the corresponding constant element from the whole constant.
> >
> > This causes the following aarch64 instruction
> > bic v4.8h, #0xf0
> > to be translated into the following op:
> > ...
> > and_vec v128,e8,tmp5,tmp4,v128$0xff0fff0fff0fff0f
> > ...
> > which is then translated to the following RISC-V instruction:
> > vand.vi v1,v1,15
> >
> > Fix that by, when VECE is 0 aka MO_8, only extracting the element if the
> > it can actually be represented as a duplicated constant.
> >
> > Fixes: 5a63f5998791 ("tcg/riscv: Add support for basic vector opcodes")
> > Signed-off-by: Aurelien Jarno <[email protected]>
> > ---
> > tcg/riscv64/tcg-target.c.inc | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > Changes v1 -> v2:
> > - Use dup_const() instead of open coding the constant duplication.
> >
> > diff --git a/tcg/riscv64/tcg-target.c.inc b/tcg/riscv64/tcg-target.c.inc
> > index 14a397117a..1262b71a62 100644
> > --- a/tcg/riscv64/tcg-target.c.inc
> > +++ b/tcg/riscv64/tcg-target.c.inc
> > @@ -392,8 +392,15 @@ static bool tcg_target_const_match(int64_t val, int ct,
> > return 1;
> > }
> > if (type >= TCG_TYPE_V64) {
> > - /* Val is replicated by VECE; extract the highest element. */
> > - val >>= (-8 << vece) & 63;
> > + /*
> > + * For vector logical operations, VECE is unused and equals to 0,
> > so
> > + * we need to check that the constant can properly be represented
> > after
> > + * replication.
> > + */
> > + if ((vece != MO_8) || (val == dup_const(vece, val))) {
> > + /* Val is replicated by VECE; extract the highest element. */
> > + val >>= (-8 << vece) & 63;
> > + }
> > }
>
> This fixes the BIC case, but I don't think it is sufficient.
>
> I tested with qemu-aarch64 built for a riscv64 host, run under
> qemu-riscv64 -cpu max, guest -cpu max,sve-default-vector-length=16.
>
> NEON bic v0.8h, #0xf0 // fail on master, now OK
>
> add z1.b; and z0.d, #0xf // FAIL
> add z1.d; and z0.b, #0xf // FAIL
> add z1.b; orr z0.d, #-16 // FAIL
> add z1.d; eor z0.b, #0xf // FAIL
This indeed doesn't work. The problem is that while the constant
matching is being passed VECE = 0 (as it is unused for logical ops), the
code generation is still being passed a 64-bit size. Therefore for the
eor, it emits:
vsetivli zero,2,e64,m1,ta,ma
lui t6,3
addiw t6,t6,-448
add t6,t6,s0
vle64.v v1,(t6)
vxor.vi v1,v1,15
It would make more sense to use 16 e8 elements like in the original ARM
SVE code, but it seems that info is lost at this stage.
> Not accepting constants for and/or/xor_vec at all
> (using C_O1_I2(v, v, v) to get them loaded into registers)
> makes all five cases pass (I'm not sure we want to keep
> the .vi forms):
>
> -- >8 --
> diff --git a/tcg/riscv64/tcg-target.c.inc b/tcg/riscv64/tcg-target.c.inc
> --- a/tcg/riscv64/tcg-target.c.inc
> +++ b/tcg/riscv64/tcg-target.c.inc
> @@ -2779,15 +2779,15 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode
> opc,
> case INDEX_op_and_vec:
> set_vtype_len(s, type);
> - tcg_out_opc_vv_vi(s, OPC_VAND_VV, OPC_VAND_VI, a0, a1, a2, c2);
> + tcg_out_opc_vv(s, OPC_VAND_VV, a0, a1, a2);
> break;
> case INDEX_op_or_vec:
> set_vtype_len(s, type);
> - tcg_out_opc_vv_vi(s, OPC_VOR_VV, OPC_VOR_VI, a0, a1, a2, c2);
> + tcg_out_opc_vv(s, OPC_VOR_VV, a0, a1, a2);
> break;
> case INDEX_op_xor_vec:
> set_vtype_len(s, type);
> - tcg_out_opc_vv_vi(s, OPC_VXOR_VV, OPC_VXOR_VI, a0, a1, a2, c2);
> + tcg_out_opc_vv(s, OPC_VXOR_VV, a0, a1, a2);
> break;
> case INDEX_op_not_vec:
> set_vtype_len(s, type);
> @@ -2976,15 +2976,15 @@ tcg_target_op_def(TCGOpcode op, TCGType type,
> unsigned flags)
> case INDEX_op_rotli_vec:
> return C_O1_I1(v, v);
> case INDEX_op_add_vec:
> - case INDEX_op_and_vec:
> - case INDEX_op_or_vec:
> - case INDEX_op_xor_vec:
> case INDEX_op_ssadd_vec:
> case INDEX_op_sssub_vec:
> case INDEX_op_usadd_vec:
> case INDEX_op_ussub_vec:
> return C_O1_I2(v, v, vK);
> case INDEX_op_sub_vec:
> return C_O1_I2(v, vK, v);
> + case INDEX_op_and_vec:
> + case INDEX_op_or_vec:
> + case INDEX_op_xor_vec:
> case INDEX_op_mul_vec:
> case INDEX_op_shlv_vec:
> case INDEX_op_shrv_vec:
> case INDEX_op_sarv_vec:
> case INDEX_op_rotlv_vec:
> case INDEX_op_rotrv_vec:
> case INDEX_op_smax_vec:
> case INDEX_op_smin_vec:
> case INDEX_op_umax_vec:
> case INDEX_op_umin_vec:
> return C_O1_I2(v, v, v);
I believe that until TCG is able to propagate VECE even for logical ops,
this is the thing to do. It generates suboptimal code, but correct one.
Therefore
Reviewed-by: Aurelien Jarno <[email protected]>
Tested-by: Aurelien Jarno <[email protected]>
Cheers
Aurelien
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
[email protected] http://aurel32.net
