* [Qemu-devel] [PATCH 0/2] tcg: tcg_can_emit_vec_op cleanup+fix @ 2018-02-17 16:40 Richard Henderson 2018-02-17 16:40 ` [Qemu-devel] [PATCH 1/2] tcg: Fold unspecified opcode test into tcg_can_emit_vec_op Richard Henderson 2018-02-17 16:40 ` [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s Richard Henderson 0 siblings, 2 replies; 6+ messages in thread From: Richard Henderson @ 2018-02-17 16:40 UTC (permalink / raw) To: qemu-devel While rearranging/rebasing my SVE patch set, I triggered an assert in tcg_gen_mul_vec. Turns out we should not have gone down that path, due to a missing tcg_can_emit_vec_op check. Tidy the usage of tcg_can_emit_vec_op to avoid code clutter. r~ Richard Henderson (2): tcg: Fold unspecified opcode test into tcg_can_emit_vec_op tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s tcg/aarch64/tcg-target.inc.c | 4 ++++ tcg/i386/tcg-target.inc.c | 4 ++++ tcg/tcg-op-gvec.c | 37 ++++++++++++++++++------------------- 3 files changed, 26 insertions(+), 19 deletions(-) -- 2.14.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] tcg: Fold unspecified opcode test into tcg_can_emit_vec_op 2018-02-17 16:40 [Qemu-devel] [PATCH 0/2] tcg: tcg_can_emit_vec_op cleanup+fix Richard Henderson @ 2018-02-17 16:40 ` Richard Henderson 2018-02-22 14:54 ` Peter Maydell 2018-02-17 16:40 ` [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s Richard Henderson 1 sibling, 1 reply; 6+ messages in thread From: Richard Henderson @ 2018-02-17 16:40 UTC (permalink / raw) To: qemu-devel This releases the callers from having to check themselves, which tidies up the code a bit. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/aarch64/tcg-target.inc.c | 4 ++++ tcg/i386/tcg-target.inc.c | 4 ++++ tcg/tcg-op-gvec.c | 28 ++++++++++++---------------- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c index be3192078d..9b0a803d79 100644 --- a/tcg/aarch64/tcg-target.inc.c +++ b/tcg/aarch64/tcg-target.inc.c @@ -2217,6 +2217,10 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc, int tcg_can_emit_vec_op(TCGOpcode opc, TCGType type, unsigned vece) { switch (opc) { + case 0: + /* Unspecified opcode */ + return 1; + case INDEX_op_add_vec: case INDEX_op_sub_vec: case INDEX_op_mul_vec: diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c index fc05909d1d..45943e540c 100644 --- a/tcg/i386/tcg-target.inc.c +++ b/tcg/i386/tcg-target.inc.c @@ -3064,6 +3064,10 @@ static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode op) int tcg_can_emit_vec_op(TCGOpcode opc, TCGType type, unsigned vece) { switch (opc) { + case 0: + /* Unspecified opcode. */ + return 1; + case INDEX_op_add_vec: case INDEX_op_sub_vec: case INDEX_op_and_vec: diff --git a/tcg/tcg-op-gvec.c b/tcg/tcg-op-gvec.c index bfe44bba81..29f9cf34b4 100644 --- a/tcg/tcg-op-gvec.c +++ b/tcg/tcg-op-gvec.c @@ -878,7 +878,7 @@ void tcg_gen_gvec_2(uint32_t dofs, uint32_t aofs, store operation. This is true for aarch64 and x86_64 hosts. */ if (TCG_TARGET_HAS_v256 && g->fniv && check_size_impl(oprsz, 32) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) { uint32_t some = QEMU_ALIGN_DOWN(oprsz, 32); expand_2_vec(g->vece, dofs, aofs, some, 32, TCG_TYPE_V256, g->fniv); if (some == oprsz) { @@ -891,12 +891,11 @@ void tcg_gen_gvec_2(uint32_t dofs, uint32_t aofs, } if (TCG_TARGET_HAS_v128 && g->fniv && check_size_impl(oprsz, 16) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) { expand_2_vec(g->vece, dofs, aofs, oprsz, 16, TCG_TYPE_V128, g->fniv); } else if (TCG_TARGET_HAS_v64 && !g->prefer_i64 && g->fniv && check_size_impl(oprsz, 8) - && (!g->opc - || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) { expand_2_vec(g->vece, dofs, aofs, oprsz, 8, TCG_TYPE_V64, g->fniv); } else if (g->fni8 && check_size_impl(oprsz, 8)) { expand_2_i64(dofs, aofs, oprsz, g->fni8); @@ -926,7 +925,7 @@ void tcg_gen_gvec_2i(uint32_t dofs, uint32_t aofs, uint32_t oprsz, that e.g. oprsz == 80 would be expanded with 2x32 + 1x16. */ if (TCG_TARGET_HAS_v256 && g->fniv && check_size_impl(oprsz, 32) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) { uint32_t some = QEMU_ALIGN_DOWN(oprsz, 32); expand_2i_vec(g->vece, dofs, aofs, some, 32, TCG_TYPE_V256, c, g->load_dest, g->fniv); @@ -940,13 +939,12 @@ void tcg_gen_gvec_2i(uint32_t dofs, uint32_t aofs, uint32_t oprsz, } if (TCG_TARGET_HAS_v128 && g->fniv && check_size_impl(oprsz, 16) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) { expand_2i_vec(g->vece, dofs, aofs, oprsz, 16, TCG_TYPE_V128, c, g->load_dest, g->fniv); } else if (TCG_TARGET_HAS_v64 && !g->prefer_i64 && g->fniv && check_size_impl(oprsz, 8) - && (!g->opc - || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) { expand_2i_vec(g->vece, dofs, aofs, oprsz, 8, TCG_TYPE_V64, c, g->load_dest, g->fniv); } else if (g->fni8 && check_size_impl(oprsz, 8)) { @@ -1063,7 +1061,7 @@ void tcg_gen_gvec_3(uint32_t dofs, uint32_t aofs, uint32_t bofs, that e.g. oprsz == 80 would be expanded with 2x32 + 1x16. */ if (TCG_TARGET_HAS_v256 && g->fniv && check_size_impl(oprsz, 32) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) { uint32_t some = QEMU_ALIGN_DOWN(oprsz, 32); expand_3_vec(g->vece, dofs, aofs, bofs, some, 32, TCG_TYPE_V256, g->load_dest, g->fniv); @@ -1078,13 +1076,12 @@ void tcg_gen_gvec_3(uint32_t dofs, uint32_t aofs, uint32_t bofs, } if (TCG_TARGET_HAS_v128 && g->fniv && check_size_impl(oprsz, 16) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) { expand_3_vec(g->vece, dofs, aofs, bofs, oprsz, 16, TCG_TYPE_V128, g->load_dest, g->fniv); } else if (TCG_TARGET_HAS_v64 && !g->prefer_i64 && g->fniv && check_size_impl(oprsz, 8) - && (!g->opc - || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) { expand_3_vec(g->vece, dofs, aofs, bofs, oprsz, 8, TCG_TYPE_V64, g->load_dest, g->fniv); } else if (g->fni8 && check_size_impl(oprsz, 8)) { @@ -1114,7 +1111,7 @@ void tcg_gen_gvec_4(uint32_t dofs, uint32_t aofs, uint32_t bofs, uint32_t cofs, that e.g. oprsz == 80 would be expanded with 2x32 + 1x16. */ if (TCG_TARGET_HAS_v256 && g->fniv && check_size_impl(oprsz, 32) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) { uint32_t some = QEMU_ALIGN_DOWN(oprsz, 32); expand_4_vec(g->vece, dofs, aofs, bofs, cofs, some, 32, TCG_TYPE_V256, g->fniv); @@ -1130,13 +1127,12 @@ void tcg_gen_gvec_4(uint32_t dofs, uint32_t aofs, uint32_t bofs, uint32_t cofs, } if (TCG_TARGET_HAS_v128 && g->fniv && check_size_impl(oprsz, 16) - && (!g->opc || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) { expand_4_vec(g->vece, dofs, aofs, bofs, cofs, oprsz, 16, TCG_TYPE_V128, g->fniv); } else if (TCG_TARGET_HAS_v64 && !g->prefer_i64 && g->fniv && check_size_impl(oprsz, 8) - && (!g->opc - || tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece))) { + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) { expand_4_vec(g->vece, dofs, aofs, bofs, cofs, oprsz, 8, TCG_TYPE_V64, g->fniv); } else if (g->fni8 && check_size_impl(oprsz, 8)) { -- 2.14.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tcg: Fold unspecified opcode test into tcg_can_emit_vec_op 2018-02-17 16:40 ` [Qemu-devel] [PATCH 1/2] tcg: Fold unspecified opcode test into tcg_can_emit_vec_op Richard Henderson @ 2018-02-22 14:54 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2018-02-22 14:54 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On 17 February 2018 at 16:40, Richard Henderson <richard.henderson@linaro.org> wrote: > This releases the callers from having to check themselves, > which tidies up the code a bit. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/aarch64/tcg-target.inc.c | 4 ++++ > tcg/i386/tcg-target.inc.c | 4 ++++ > tcg/tcg-op-gvec.c | 28 ++++++++++++---------------- > 3 files changed, 20 insertions(+), 16 deletions(-) > > diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c > index be3192078d..9b0a803d79 100644 > --- a/tcg/aarch64/tcg-target.inc.c > +++ b/tcg/aarch64/tcg-target.inc.c > @@ -2217,6 +2217,10 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc, > int tcg_can_emit_vec_op(TCGOpcode opc, TCGType type, unsigned vece) > { > switch (opc) { > + case 0: > + /* Unspecified opcode */ > + return 1; > + > case INDEX_op_add_vec: > case INDEX_op_sub_vec: > case INDEX_op_mul_vec: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> since there's clearly no behaviour change here. But it isn't clear to me why tcg_can_emit_vec_op() should return 1 for unspecified opcodes -- shouldn't the default be "we can't vectorize something we don't know about", not "no idea what this is so let's assume it's vectorizable" ? A comment about why unspecified opcodes are always OK might be helpful. thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s 2018-02-17 16:40 [Qemu-devel] [PATCH 0/2] tcg: tcg_can_emit_vec_op cleanup+fix Richard Henderson 2018-02-17 16:40 ` [Qemu-devel] [PATCH 1/2] tcg: Fold unspecified opcode test into tcg_can_emit_vec_op Richard Henderson @ 2018-02-17 16:40 ` Richard Henderson 2018-02-22 14:58 ` Peter Maydell 1 sibling, 1 reply; 6+ messages in thread From: Richard Henderson @ 2018-02-17 16:40 UTC (permalink / raw) To: qemu-devel This lead to an assertion failure for 64-bit vector multiply, which is not available in the AVX instruction set. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/tcg-op-gvec.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tcg/tcg-op-gvec.c b/tcg/tcg-op-gvec.c index 29f9cf34b4..432e577c35 100644 --- a/tcg/tcg-op-gvec.c +++ b/tcg/tcg-op-gvec.c @@ -979,12 +979,15 @@ void tcg_gen_gvec_2s(uint32_t dofs, uint32_t aofs, uint32_t oprsz, type = 0; if (g->fniv) { - if (TCG_TARGET_HAS_v256 && check_size_impl(oprsz, 32)) { + if (TCG_TARGET_HAS_v256 && check_size_impl(oprsz, 32) + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) { type = TCG_TYPE_V256; - } else if (TCG_TARGET_HAS_v128 && check_size_impl(oprsz, 16)) { + } else if (TCG_TARGET_HAS_v128 && check_size_impl(oprsz, 16) + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) { type = TCG_TYPE_V128; } else if (TCG_TARGET_HAS_v64 && !g->prefer_i64 - && check_size_impl(oprsz, 8)) { + && check_size_impl(oprsz, 8) + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) { type = TCG_TYPE_V64; } } -- 2.14.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s 2018-02-17 16:40 ` [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s Richard Henderson @ 2018-02-22 14:58 ` Peter Maydell 2018-02-22 15:32 ` Richard Henderson 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2018-02-22 14:58 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On 17 February 2018 at 16:40, Richard Henderson <richard.henderson@linaro.org> wrote: > This lead to an assertion failure for 64-bit vector multiply, > which is not available in the AVX instruction set. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/tcg-op-gvec.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/tcg/tcg-op-gvec.c b/tcg/tcg-op-gvec.c > index 29f9cf34b4..432e577c35 100644 > --- a/tcg/tcg-op-gvec.c > +++ b/tcg/tcg-op-gvec.c > @@ -979,12 +979,15 @@ void tcg_gen_gvec_2s(uint32_t dofs, uint32_t aofs, uint32_t oprsz, > > type = 0; > if (g->fniv) { > - if (TCG_TARGET_HAS_v256 && check_size_impl(oprsz, 32)) { > + if (TCG_TARGET_HAS_v256 && check_size_impl(oprsz, 32) > + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) { > type = TCG_TYPE_V256; > - } else if (TCG_TARGET_HAS_v128 && check_size_impl(oprsz, 16)) { > + } else if (TCG_TARGET_HAS_v128 && check_size_impl(oprsz, 16) > + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) { > type = TCG_TYPE_V128; > } else if (TCG_TARGET_HAS_v64 && !g->prefer_i64 > - && check_size_impl(oprsz, 8)) { > + && check_size_impl(oprsz, 8) > + && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) { > type = TCG_TYPE_V64; > } > } Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Incidentally, I notice that the condition checks (TCG_TARGET_HAS_v256 && check_size_impl(oprsz, 32) && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) (TCG_TARGET_HAS_v128 && check_size_impl(oprsz, 16) && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) (TCG_TARGET_HAS_v64 && check_size_impl(oprsz, 8) && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) seem to be quite commonly used -- perhaps factoring these out into suitably named functions would make the code more readable? thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s 2018-02-22 14:58 ` Peter Maydell @ 2018-02-22 15:32 ` Richard Henderson 0 siblings, 0 replies; 6+ messages in thread From: Richard Henderson @ 2018-02-22 15:32 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers On 02/22/2018 06:58 AM, Peter Maydell wrote: > Incidentally, I notice that the condition checks > (TCG_TARGET_HAS_v256 && check_size_impl(oprsz, 32) > && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V256, g->vece)) > > (TCG_TARGET_HAS_v128 && check_size_impl(oprsz, 16) > && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V128, g->vece)) > > (TCG_TARGET_HAS_v64 && check_size_impl(oprsz, 8) > && tcg_can_emit_vec_op(g->opc, TCG_TYPE_V64, g->vece)) > > seem to be quite commonly used -- perhaps factoring these out > into suitably named functions would make the code more readable? That's a good idea. I'll see what I can come up with there. r~ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-22 15:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-17 16:40 [Qemu-devel] [PATCH 0/2] tcg: tcg_can_emit_vec_op cleanup+fix Richard Henderson 2018-02-17 16:40 ` [Qemu-devel] [PATCH 1/2] tcg: Fold unspecified opcode test into tcg_can_emit_vec_op Richard Henderson 2018-02-22 14:54 ` Peter Maydell 2018-02-17 16:40 ` [Qemu-devel] [PATCH 2/2] tcg: Add missing tcg_can_emit_vec_op check in tcg_gen_gvec_2s Richard Henderson 2018-02-22 14:58 ` Peter Maydell 2018-02-22 15:32 ` Richard Henderson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.