From: Pierrick Bouvier <pierrick.bouvier@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>, qemu-devel@nongnu.org
Cc: alex.bennee@linaro.org, Elisha Hollander <just4now666666@gmail.com>
Subject: Re: [PATCH 2/2] tcg: Propagate new TCGOp to add_as_label_use
Date: Tue, 10 Sep 2024 14:50:34 -0700 [thread overview]
Message-ID: <047bf152-ac20-49e0-90fb-ff9b56511982@linaro.org> (raw)
In-Reply-To: <20240910212351.977753-3-richard.henderson@linaro.org>
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>
next prev parent reply other threads:[~2024-09-10 21:50 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=047bf152-ac20-49e0-90fb-ff9b56511982@linaro.org \
--to=pierrick.bouvier@linaro.org \
--cc=alex.bennee@linaro.org \
--cc=just4now666666@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).