* [PATCH 0/2] tcg: Fix branch/label link during plugin expansion
@ 2024-09-10 21:23 Richard Henderson
2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Richard Henderson @ 2024-09-10 21:23 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, pierrick.bouvier
With tcg_last_op(), we always get the last op of the stream.
With TCGContext.emit_before_op, the most recently emitted op
is no longer the last op.
Instead, pass the op being emitted back from the allocator so
that we can link it to the label without needing to look it up.
r~
Richard Henderson (2):
tcg: Return TCGOp from tcg_gen_op[1-6]
tcg: Propagate new TCGOp to add_as_label_use
tcg/tcg-internal.h | 12 +++----
tcg/tcg-op.c | 86 +++++++++++++++++++++++++---------------------
2 files changed, 53 insertions(+), 45 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] 2024-09-10 21:23 [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson @ 2024-09-10 21:23 ` Richard Henderson 2024-09-10 21:45 ` Pierrick Bouvier ` (2 more replies) 2024-09-10 21:23 ` [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use Richard Henderson 2024-09-10 21:28 ` [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson 2 siblings, 3 replies; 12+ messages in thread From: Richard Henderson @ 2024-09-10 21:23 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, pierrick.bouvier Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/tcg-internal.h | 12 ++++++------ tcg/tcg-op.c | 23 +++++++++++++++-------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/tcg/tcg-internal.h b/tcg/tcg-internal.h index 52103f4164..8099248076 100644 --- a/tcg/tcg-internal.h +++ b/tcg/tcg-internal.h @@ -92,12 +92,12 @@ TCGTemp *tcg_temp_new_internal(TCGType type, TCGTempKind kind); */ TCGTemp *tcg_constant_internal(TCGType type, int64_t val); -void tcg_gen_op1(TCGOpcode, TCGArg); -void tcg_gen_op2(TCGOpcode, TCGArg, TCGArg); -void tcg_gen_op3(TCGOpcode, TCGArg, TCGArg, TCGArg); -void tcg_gen_op4(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg); -void tcg_gen_op5(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); -void tcg_gen_op6(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); +TCGOp *tcg_gen_op1(TCGOpcode, TCGArg); +TCGOp *tcg_gen_op2(TCGOpcode, TCGArg, TCGArg); +TCGOp *tcg_gen_op3(TCGOpcode, TCGArg, TCGArg, TCGArg); +TCGOp *tcg_gen_op4(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg); +TCGOp *tcg_gen_op5(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); +TCGOp *tcg_gen_op6(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); void vec_gen_2(TCGOpcode, TCGType, unsigned, TCGArg, TCGArg); void vec_gen_3(TCGOpcode, TCGType, unsigned, TCGArg, TCGArg, TCGArg); diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c index eff3728622..28c41b37a4 100644 --- a/tcg/tcg-op.c +++ b/tcg/tcg-op.c @@ -37,38 +37,43 @@ */ #define NI __attribute__((noinline)) -void NI tcg_gen_op1(TCGOpcode opc, TCGArg a1) +TCGOp * NI tcg_gen_op1(TCGOpcode opc, TCGArg a1) { TCGOp *op = tcg_emit_op(opc, 1); op->args[0] = a1; + return op; } -void NI tcg_gen_op2(TCGOpcode opc, TCGArg a1, TCGArg a2) +TCGOp * NI tcg_gen_op2(TCGOpcode opc, TCGArg a1, TCGArg a2) { TCGOp *op = tcg_emit_op(opc, 2); op->args[0] = a1; op->args[1] = a2; + return op; } -void NI tcg_gen_op3(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3) +TCGOp * NI tcg_gen_op3(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3) { TCGOp *op = tcg_emit_op(opc, 3); op->args[0] = a1; op->args[1] = a2; op->args[2] = a3; + return op; } -void NI tcg_gen_op4(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, TCGArg a4) +TCGOp * NI tcg_gen_op4(TCGOpcode opc, TCGArg a1, TCGArg a2, + TCGArg a3, TCGArg a4) { TCGOp *op = tcg_emit_op(opc, 4); op->args[0] = a1; op->args[1] = a2; op->args[2] = a3; op->args[3] = a4; + return op; } -void NI tcg_gen_op5(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, - TCGArg a4, TCGArg a5) +TCGOp * NI tcg_gen_op5(TCGOpcode opc, TCGArg a1, TCGArg a2, + TCGArg a3, TCGArg a4, TCGArg a5) { TCGOp *op = tcg_emit_op(opc, 5); op->args[0] = a1; @@ -76,10 +81,11 @@ void NI tcg_gen_op5(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, op->args[2] = a3; op->args[3] = a4; op->args[4] = a5; + return op; } -void NI tcg_gen_op6(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, - TCGArg a4, TCGArg a5, TCGArg a6) +TCGOp * NI tcg_gen_op6(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, + TCGArg a4, TCGArg a5, TCGArg a6) { TCGOp *op = tcg_emit_op(opc, 6); op->args[0] = a1; @@ -88,6 +94,7 @@ void NI tcg_gen_op6(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, op->args[3] = a4; op->args[4] = a5; op->args[5] = a6; + return op; } /* -- 2.43.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] 2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson @ 2024-09-10 21:45 ` Pierrick Bouvier 2024-09-13 10:26 ` Alex Bennée 2024-09-13 20:50 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 12+ messages in thread From: Pierrick Bouvier @ 2024-09-10 21:45 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: alex.bennee On 9/10/24 14:23, Richard Henderson wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/tcg-internal.h | 12 ++++++------ > tcg/tcg-op.c | 23 +++++++++++++++-------- > 2 files changed, 21 insertions(+), 14 deletions(-) > > diff --git a/tcg/tcg-internal.h b/tcg/tcg-internal.h > index 52103f4164..8099248076 100644 > --- a/tcg/tcg-internal.h > +++ b/tcg/tcg-internal.h > @@ -92,12 +92,12 @@ TCGTemp *tcg_temp_new_internal(TCGType type, TCGTempKind kind); > */ > TCGTemp *tcg_constant_internal(TCGType type, int64_t val); > > -void tcg_gen_op1(TCGOpcode, TCGArg); > -void tcg_gen_op2(TCGOpcode, TCGArg, TCGArg); > -void tcg_gen_op3(TCGOpcode, TCGArg, TCGArg, TCGArg); > -void tcg_gen_op4(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg); > -void tcg_gen_op5(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); > -void tcg_gen_op6(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); > +TCGOp *tcg_gen_op1(TCGOpcode, TCGArg); > +TCGOp *tcg_gen_op2(TCGOpcode, TCGArg, TCGArg); > +TCGOp *tcg_gen_op3(TCGOpcode, TCGArg, TCGArg, TCGArg); > +TCGOp *tcg_gen_op4(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg); > +TCGOp *tcg_gen_op5(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); > +TCGOp *tcg_gen_op6(TCGOpcode, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg, TCGArg); > > void vec_gen_2(TCGOpcode, TCGType, unsigned, TCGArg, TCGArg); > void vec_gen_3(TCGOpcode, TCGType, unsigned, TCGArg, TCGArg, TCGArg); > diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c > index eff3728622..28c41b37a4 100644 > --- a/tcg/tcg-op.c > +++ b/tcg/tcg-op.c > @@ -37,38 +37,43 @@ > */ > #define NI __attribute__((noinline)) > > -void NI tcg_gen_op1(TCGOpcode opc, TCGArg a1) > +TCGOp * NI tcg_gen_op1(TCGOpcode opc, TCGArg a1) > { > TCGOp *op = tcg_emit_op(opc, 1); > op->args[0] = a1; > + return op; > } > > -void NI tcg_gen_op2(TCGOpcode opc, TCGArg a1, TCGArg a2) > +TCGOp * NI tcg_gen_op2(TCGOpcode opc, TCGArg a1, TCGArg a2) > { > TCGOp *op = tcg_emit_op(opc, 2); > op->args[0] = a1; > op->args[1] = a2; > + return op; > } > > -void NI tcg_gen_op3(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3) > +TCGOp * NI tcg_gen_op3(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3) > { > TCGOp *op = tcg_emit_op(opc, 3); > op->args[0] = a1; > op->args[1] = a2; > op->args[2] = a3; > + return op; > } > > -void NI tcg_gen_op4(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, TCGArg a4) > +TCGOp * NI tcg_gen_op4(TCGOpcode opc, TCGArg a1, TCGArg a2, > + TCGArg a3, TCGArg a4) > { > TCGOp *op = tcg_emit_op(opc, 4); > op->args[0] = a1; > op->args[1] = a2; > op->args[2] = a3; > op->args[3] = a4; > + return op; > } > > -void NI tcg_gen_op5(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, > - TCGArg a4, TCGArg a5) > +TCGOp * NI tcg_gen_op5(TCGOpcode opc, TCGArg a1, TCGArg a2, > + TCGArg a3, TCGArg a4, TCGArg a5) > { > TCGOp *op = tcg_emit_op(opc, 5); > op->args[0] = a1; > @@ -76,10 +81,11 @@ void NI tcg_gen_op5(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, > op->args[2] = a3; > op->args[3] = a4; > op->args[4] = a5; > + return op; > } > > -void NI tcg_gen_op6(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, > - TCGArg a4, TCGArg a5, TCGArg a6) > +TCGOp * NI tcg_gen_op6(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, > + TCGArg a4, TCGArg a5, TCGArg a6) > { > TCGOp *op = tcg_emit_op(opc, 6); > op->args[0] = a1; > @@ -88,6 +94,7 @@ void NI tcg_gen_op6(TCGOpcode opc, TCGArg a1, TCGArg a2, TCGArg a3, > op->args[3] = a4; > op->args[4] = a5; > op->args[5] = a6; > + return op; > } > > /* Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] 2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson 2024-09-10 21:45 ` Pierrick Bouvier @ 2024-09-13 10:26 ` Alex Bennée 2024-09-13 20:50 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 12+ messages in thread From: Alex Bennée @ 2024-09-13 10:26 UTC (permalink / raw) To: Richard Henderson; +Cc: qemu-devel, pierrick.bouvier Richard Henderson <richard.henderson@linaro.org> writes: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée Virtualisation Tech Lead @ Linaro ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] 2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson 2024-09-10 21:45 ` Pierrick Bouvier 2024-09-13 10:26 ` Alex Bennée @ 2024-09-13 20:50 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2024-09-13 20:50 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: alex.bennee, pierrick.bouvier Maybe a line like "See the justification in the next commit"? On 10/9/24 23:23, Richard Henderson wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/tcg-internal.h | 12 ++++++------ > tcg/tcg-op.c | 23 +++++++++++++++-------- > 2 files changed, 21 insertions(+), 14 deletions(-) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use 2024-09-10 21:23 [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson 2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson @ 2024-09-10 21:23 ` Richard Henderson 2024-09-10 21:50 ` Pierrick Bouvier 2024-09-10 21:28 ` [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson 2 siblings, 1 reply; 12+ messages in thread From: Richard Henderson @ 2024-09-10 21:23 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, pierrick.bouvier, Elisha Hollander The use of tcg_last_op does not interact well with TCGContext.emit_before_op, resulting in the label being linked to something other than the branch op. In this case it is easier to simply collect the emitted branch op and pass it directly to add_as_label_use. Reported-by: Elisha Hollander <just4now666666@gmail.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/tcg-op.c | 63 ++++++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c index 28c41b37a4..4a7e705367 100644 --- a/tcg/tcg-op.c +++ b/tcg/tcg-op.c @@ -117,9 +117,9 @@ static void DNI tcg_gen_op1_i64(TCGOpcode opc, TCGv_i64 a1) tcg_gen_op1(opc, tcgv_i64_arg(a1)); } -static void DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1) +static TCGOp * DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1) { - tcg_gen_op1(opc, a1); + return tcg_gen_op1(opc, a1); } static void DNI tcg_gen_op2_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2) @@ -196,16 +196,16 @@ static void DNI tcg_gen_op4i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, tcgv_i64_arg(a3), a4); } -static void DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, - TCGArg a3, TCGArg a4) +static TCGOp * DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, + TCGArg a3, TCGArg a4) { - tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4); + return tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4); } -static void DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, - TCGArg a3, TCGArg a4) +static TCGOp * DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, + TCGArg a3, TCGArg a4) { - tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4); + return tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4); } static void DNI tcg_gen_op5_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, @@ -270,12 +270,12 @@ static void DNI tcg_gen_op6i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, tcgv_i64_arg(a3), tcgv_i64_arg(a4), tcgv_i64_arg(a5), a6); } -static void DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, - TCGv_i32 a3, TCGv_i32 a4, - TCGArg a5, TCGArg a6) +static TCGOp * DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, + TCGv_i32 a3, TCGv_i32 a4, + TCGArg a5, TCGArg a6) { - tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), - tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6); + return tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), + tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6); } /* Generic ops. */ @@ -286,18 +286,17 @@ void gen_set_label(TCGLabel *l) tcg_gen_op1(INDEX_op_set_label, label_arg(l)); } -static void add_last_as_label_use(TCGLabel *l) +static void add_as_label_use(TCGLabel *l, TCGOp *op) { TCGLabelUse *u = tcg_malloc(sizeof(TCGLabelUse)); - u->op = tcg_last_op(); + u->op = op; QSIMPLEQ_INSERT_TAIL(&l->branches, u, next); } void tcg_gen_br(TCGLabel *l) { - tcg_gen_op1(INDEX_op_br, label_arg(l)); - add_last_as_label_use(l); + add_as_label_use(l, tcg_gen_op1(INDEX_op_br, label_arg(l))); } void tcg_gen_mb(TCGBar mb_type) @@ -514,8 +513,9 @@ void tcg_gen_brcond_i32(TCGCond cond, TCGv_i32 arg1, TCGv_i32 arg2, TCGLabel *l) if (cond == TCG_COND_ALWAYS) { tcg_gen_br(l); } else if (cond != TCG_COND_NEVER) { - tcg_gen_op4ii_i32(INDEX_op_brcond_i32, arg1, arg2, cond, label_arg(l)); - add_last_as_label_use(l); + TCGOp *op = tcg_gen_op4ii_i32(INDEX_op_brcond_i32, + arg1, arg2, cond, label_arg(l)); + add_as_label_use(l, op); } } @@ -1934,15 +1934,16 @@ void tcg_gen_brcond_i64(TCGCond cond, TCGv_i64 arg1, TCGv_i64 arg2, TCGLabel *l) if (cond == TCG_COND_ALWAYS) { tcg_gen_br(l); } else if (cond != TCG_COND_NEVER) { + TCGOp *op; if (TCG_TARGET_REG_BITS == 32) { - tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1), - TCGV_HIGH(arg1), TCGV_LOW(arg2), - TCGV_HIGH(arg2), cond, label_arg(l)); + op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1), + TCGV_HIGH(arg1), TCGV_LOW(arg2), + TCGV_HIGH(arg2), cond, label_arg(l)); } else { - tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond, - label_arg(l)); + op = tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond, + label_arg(l)); } - add_last_as_label_use(l); + add_as_label_use(l, op); } } @@ -1953,12 +1954,12 @@ void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, TCGLabel *l) } else if (cond == TCG_COND_ALWAYS) { tcg_gen_br(l); } else if (cond != TCG_COND_NEVER) { - tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, - TCGV_LOW(arg1), TCGV_HIGH(arg1), - tcg_constant_i32(arg2), - tcg_constant_i32(arg2 >> 32), - cond, label_arg(l)); - add_last_as_label_use(l); + TCGOp *op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, + TCGV_LOW(arg1), TCGV_HIGH(arg1), + tcg_constant_i32(arg2), + tcg_constant_i32(arg2 >> 32), + cond, label_arg(l)); + add_as_label_use(l, op); } } -- 2.43.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use 2024-09-10 21:23 ` [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use Richard Henderson @ 2024-09-10 21:50 ` Pierrick Bouvier 2024-09-10 21:56 ` Richard Henderson 0 siblings, 1 reply; 12+ messages in thread From: Pierrick Bouvier @ 2024-09-10 21:50 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: alex.bennee, Elisha Hollander On 9/10/24 14:23, Richard Henderson wrote: > The use of tcg_last_op does not interact well with > TCGContext.emit_before_op, resulting in the label > being linked to something other than the branch op. > > In this case it is easier to simply collect the emitted > branch op and pass it directly to add_as_label_use. > > Reported-by: Elisha Hollander <just4now666666@gmail.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/tcg-op.c | 63 ++++++++++++++++++++++++++-------------------------- > 1 file changed, 32 insertions(+), 31 deletions(-) > > diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c > index 28c41b37a4..4a7e705367 100644 > --- a/tcg/tcg-op.c > +++ b/tcg/tcg-op.c > @@ -117,9 +117,9 @@ static void DNI tcg_gen_op1_i64(TCGOpcode opc, TCGv_i64 a1) > tcg_gen_op1(opc, tcgv_i64_arg(a1)); > } > > -static void DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1) > +static TCGOp * DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1) > { > - tcg_gen_op1(opc, a1); > + return tcg_gen_op1(opc, a1); > } > > static void DNI tcg_gen_op2_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2) > @@ -196,16 +196,16 @@ static void DNI tcg_gen_op4i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, > tcgv_i64_arg(a3), a4); > } > > -static void DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, > - TCGArg a3, TCGArg a4) > +static TCGOp * DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, > + TCGArg a3, TCGArg a4) > { > - tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4); > + return tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4); > } > > -static void DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, > - TCGArg a3, TCGArg a4) > +static TCGOp * DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, > + TCGArg a3, TCGArg a4) > { > - tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4); > + return tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4); > } > > static void DNI tcg_gen_op5_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, > @@ -270,12 +270,12 @@ static void DNI tcg_gen_op6i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, > tcgv_i64_arg(a3), tcgv_i64_arg(a4), tcgv_i64_arg(a5), a6); > } > > -static void DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, > - TCGv_i32 a3, TCGv_i32 a4, > - TCGArg a5, TCGArg a6) > +static TCGOp * DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2, > + TCGv_i32 a3, TCGv_i32 a4, > + TCGArg a5, TCGArg a6) > { > - tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), > - tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6); > + return tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), > + tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6); > } > > /* Generic ops. */ > @@ -286,18 +286,17 @@ void gen_set_label(TCGLabel *l) > tcg_gen_op1(INDEX_op_set_label, label_arg(l)); > } > > -static void add_last_as_label_use(TCGLabel *l) > +static void add_as_label_use(TCGLabel *l, TCGOp *op) > { > TCGLabelUse *u = tcg_malloc(sizeof(TCGLabelUse)); > > - u->op = tcg_last_op(); > + u->op = op; > QSIMPLEQ_INSERT_TAIL(&l->branches, u, next); > } > > void tcg_gen_br(TCGLabel *l) > { > - tcg_gen_op1(INDEX_op_br, label_arg(l)); > - add_last_as_label_use(l); > + add_as_label_use(l, tcg_gen_op1(INDEX_op_br, label_arg(l))); > } > > void tcg_gen_mb(TCGBar mb_type) > @@ -514,8 +513,9 @@ void tcg_gen_brcond_i32(TCGCond cond, TCGv_i32 arg1, TCGv_i32 arg2, TCGLabel *l) > if (cond == TCG_COND_ALWAYS) { > tcg_gen_br(l); > } else if (cond != TCG_COND_NEVER) { > - tcg_gen_op4ii_i32(INDEX_op_brcond_i32, arg1, arg2, cond, label_arg(l)); > - add_last_as_label_use(l); > + TCGOp *op = tcg_gen_op4ii_i32(INDEX_op_brcond_i32, > + arg1, arg2, cond, label_arg(l)); > + add_as_label_use(l, op); > } > } > > @@ -1934,15 +1934,16 @@ void tcg_gen_brcond_i64(TCGCond cond, TCGv_i64 arg1, TCGv_i64 arg2, TCGLabel *l) > if (cond == TCG_COND_ALWAYS) { > tcg_gen_br(l); > } else if (cond != TCG_COND_NEVER) { > + TCGOp *op; > if (TCG_TARGET_REG_BITS == 32) { > - tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1), > - TCGV_HIGH(arg1), TCGV_LOW(arg2), > - TCGV_HIGH(arg2), cond, label_arg(l)); > + op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1), > + TCGV_HIGH(arg1), TCGV_LOW(arg2), > + TCGV_HIGH(arg2), cond, label_arg(l)); > } else { > - tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond, > - label_arg(l)); > + op = tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond, > + label_arg(l)); > } > - add_last_as_label_use(l); > + add_as_label_use(l, op); > } > } > > @@ -1953,12 +1954,12 @@ void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, TCGLabel *l) > } else if (cond == TCG_COND_ALWAYS) { > tcg_gen_br(l); > } else if (cond != TCG_COND_NEVER) { > - tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, > - TCGV_LOW(arg1), TCGV_HIGH(arg1), > - tcg_constant_i32(arg2), > - tcg_constant_i32(arg2 >> 32), > - cond, label_arg(l)); > - add_last_as_label_use(l); > + TCGOp *op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, > + TCGV_LOW(arg1), TCGV_HIGH(arg1), > + tcg_constant_i32(arg2), > + tcg_constant_i32(arg2 >> 32), > + cond, label_arg(l)); > + add_as_label_use(l, op); > } > } > the tcg_last_op() referred to in this case can be another op than the one expected? Are there other cases where usage of tcg_last_op could be a problem? Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use 2024-09-10 21:50 ` Pierrick Bouvier @ 2024-09-10 21:56 ` Richard Henderson 0 siblings, 0 replies; 12+ messages in thread From: Richard Henderson @ 2024-09-10 21:56 UTC (permalink / raw) To: Pierrick Bouvier, qemu-devel; +Cc: alex.bennee, Elisha Hollander On 9/10/24 14:50, Pierrick Bouvier wrote: > the tcg_last_op() referred to in this case can be another op than the one expected? > Are there other cases where usage of tcg_last_op could be a problem? I don't think so. Aside from plugins, there are (currently) no other uses of emit_before_op. r~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcg: Fix branch/label link during plugin expansion 2024-09-10 21:23 [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson 2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson 2024-09-10 21:23 ` [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use Richard Henderson @ 2024-09-10 21:28 ` Richard Henderson 2024-09-13 10:23 ` Alex Bennée 2 siblings, 1 reply; 12+ messages in thread From: Richard Henderson @ 2024-09-10 21:28 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, pierrick.bouvier On 9/10/24 14:23, Richard Henderson wrote: > With tcg_last_op(), we always get the last op of the stream. > With TCGContext.emit_before_op, the most recently emitted op > is no longer the last op. > > Instead, pass the op being emitted back from the allocator so > that we can link it to the label without needing to look it up. Oh, I meant to point out from whence this comes. The plugin uses a conditional ld_i32 tmp18,env,$0xffffffffffffdb10 mul_i32 tmp18,tmp18,$0x18 ext_i32_i64 tmp17,tmp18 add_i64 tmp17,tmp17,$0x575410edadc8 ld_i64 tmp21,tmp17,$0x0 brcond_i64 tmp21,$0x0,ltu,$L1 ld_i32 tmp18,env,$0xffffffffffffdb10 call plugin(0x79a2abfde66a),$0x1,$0,tmp18,$0x0 set_label $L1 Note that the branch is X < 0 (unsigned), which is always false, and thus the branch is optimized away. r~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcg: Fix branch/label link during plugin expansion 2024-09-10 21:28 ` [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson @ 2024-09-13 10:23 ` Alex Bennée 2024-09-13 16:27 ` Richard Henderson 2024-09-18 18:43 ` Pierrick Bouvier 0 siblings, 2 replies; 12+ messages in thread From: Alex Bennée @ 2024-09-13 10:23 UTC (permalink / raw) To: Richard Henderson; +Cc: qemu-devel, pierrick.bouvier Richard Henderson <richard.henderson@linaro.org> writes: > On 9/10/24 14:23, Richard Henderson wrote: >> With tcg_last_op(), we always get the last op of the stream. >> With TCGContext.emit_before_op, the most recently emitted op >> is no longer the last op. >> Instead, pass the op being emitted back from the allocator so >> that we can link it to the label without needing to look it up. > > Oh, I meant to point out from whence this comes. > The plugin uses a conditional size_t n_insns = qemu_plugin_tb_n_insns(tb); qemu_plugin_u64 quantum_insn = qemu_plugin_scoreboard_u64_in_struct(vcpus, vCPUTime, quantum_insn); /* count (and eventually trap) once per tb */ qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu( tb, QEMU_PLUGIN_INLINE_ADD_U64, quantum_insn, n_insns); > ld_i32 tmp18,env,$0xffffffffffffdb10 > mul_i32 tmp18,tmp18,$0x18 > ext_i32_i64 tmp17,tmp18 > add_i64 tmp17,tmp17,$0x575410edadc8 qemu_plugin_register_vcpu_tb_exec_cond_cb( tb, every_quantum_insn, QEMU_PLUGIN_CB_NO_REGS, QEMU_PLUGIN_COND_GE, quantum_insn, max_insn_per_quantum, NULL); ? > ld_i64 tmp21,tmp17,$0x0 > brcond_i64 tmp21,$0x0,ltu,$L1 > ld_i32 tmp18,env,$0xffffffffffffdb10 > call plugin(0x79a2abfde66a),$0x1,$0,tmp18,$0x0 > set_label $L1 > > Note that the branch is X < 0 (unsigned), which is always false, and > thus the branch is optimized away. I'm obviously missing something reading this. How can TCG know the state of the scoreboard variables and optimise away the branch? > > > r~ -- Alex Bennée Virtualisation Tech Lead @ Linaro ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcg: Fix branch/label link during plugin expansion 2024-09-13 10:23 ` Alex Bennée @ 2024-09-13 16:27 ` Richard Henderson 2024-09-18 18:43 ` Pierrick Bouvier 1 sibling, 0 replies; 12+ messages in thread From: Richard Henderson @ 2024-09-13 16:27 UTC (permalink / raw) To: Alex Bennée; +Cc: qemu-devel, pierrick.bouvier On 9/13/24 03:23, Alex Bennée wrote: >> Note that the branch is X < 0 (unsigned), which is always false, and >> thus the branch is optimized away. > > I'm obviously missing something reading this. How can TCG know the state > of the scoreboard variables and optimise away the branch? 0 < 0 is of course false. r~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] tcg: Fix branch/label link during plugin expansion 2024-09-13 10:23 ` Alex Bennée 2024-09-13 16:27 ` Richard Henderson @ 2024-09-18 18:43 ` Pierrick Bouvier 1 sibling, 0 replies; 12+ messages in thread From: Pierrick Bouvier @ 2024-09-18 18:43 UTC (permalink / raw) To: Alex Bennée, Richard Henderson; +Cc: qemu-devel On 9/13/24 03:23, Alex Bennée wrote: > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 9/10/24 14:23, Richard Henderson wrote: >>> With tcg_last_op(), we always get the last op of the stream. >>> With TCGContext.emit_before_op, the most recently emitted op >>> is no longer the last op. >>> Instead, pass the op being emitted back from the allocator so >>> that we can link it to the label without needing to look it up. >> >> Oh, I meant to point out from whence this comes. >> The plugin uses a conditional > > size_t n_insns = qemu_plugin_tb_n_insns(tb); > qemu_plugin_u64 quantum_insn = > qemu_plugin_scoreboard_u64_in_struct(vcpus, vCPUTime, quantum_insn); > /* count (and eventually trap) once per tb */ > qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu( > tb, QEMU_PLUGIN_INLINE_ADD_U64, quantum_insn, n_insns); > >> ld_i32 tmp18,env,$0xffffffffffffdb10 >> mul_i32 tmp18,tmp18,$0x18 >> ext_i32_i64 tmp17,tmp18 >> add_i64 tmp17,tmp17,$0x575410edadc8 > > qemu_plugin_register_vcpu_tb_exec_cond_cb( > tb, every_quantum_insn, > QEMU_PLUGIN_CB_NO_REGS, QEMU_PLUGIN_COND_GE, > quantum_insn, max_insn_per_quantum, NULL); > > ? > >> ld_i64 tmp21,tmp17,$0x0 >> brcond_i64 tmp21,$0x0,ltu,$L1 >> ld_i32 tmp18,env,$0xffffffffffffdb10 >> call plugin(0x79a2abfde66a),$0x1,$0,tmp18,$0x0 >> set_label $L1 >> >> Note that the branch is X < 0 (unsigned), which is always false, and >> thus the branch is optimized away. > > I'm obviously missing something reading this. How can TCG know the state > of the scoreboard variables and optimise away the branch? > The constant against which we compare scoreboard entry value is known at translation time. >> >> >> r~ > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-09-18 18:44 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-10 21:23 [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson 2024-09-10 21:23 ` [PATCH 1/2] tcg: Return TCGOp from tcg_gen_op[1-6] Richard Henderson 2024-09-10 21:45 ` Pierrick Bouvier 2024-09-13 10:26 ` Alex Bennée 2024-09-13 20:50 ` Philippe Mathieu-Daudé 2024-09-10 21:23 ` [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use Richard Henderson 2024-09-10 21:50 ` Pierrick Bouvier 2024-09-10 21:56 ` Richard Henderson 2024-09-10 21:28 ` [PATCH 0/2] tcg: Fix branch/label link during plugin expansion Richard Henderson 2024-09-13 10:23 ` Alex Bennée 2024-09-13 16:27 ` Richard Henderson 2024-09-18 18:43 ` Pierrick Bouvier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).