* [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
* [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 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
* 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.