From: Richard Henderson <richard.henderson@linaro.org>
To: Jiaxun Yang <jiaxun.yang@flygoat.com>, qemu-devel@nongnu.org
Cc: "Song Gao" <gaosong@loongson.cn>,
"Bibo Mao" <maobibo@loongson.cn>,
"Eric Blake" <eblake@redhat.com>,
"Markus Armbruster" <armbru@redhat.com>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Yanan Wang" <wangyanan55@huawei.com>,
"Zhao Liu" <zhao1.liu@intel.com>,
"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH v2 12/23] target/loongarch: Scrutinise TCG bitops translation for 32 bit build
Date: Thu, 26 Dec 2024 13:55:20 -0800 [thread overview]
Message-ID: <f2360c0b-979a-4473-a7b1-96caa54cff27@linaro.org> (raw)
In-Reply-To: <20241226-la32-fixes1-v2-12-0414594f8cb5@flygoat.com>
On 12/26/24 13:19, Jiaxun Yang wrote:
> Use tl variant whenever possible.
>
> Silent compiler warnings by performing casting for come consts.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> target/loongarch/tcg/insn_trans/trans_bit.c.inc | 34 ++++++++++++++-----------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/target/loongarch/tcg/insn_trans/trans_bit.c.inc b/target/loongarch/tcg/insn_trans/trans_bit.c.inc
> index ee5fa003ce06a1910f826c3eb96d1d532c32e02c..a40346a670be31a123848e8ea5f7b94f8372976b 100644
> --- a/target/loongarch/tcg/insn_trans/trans_bit.c.inc
> +++ b/target/loongarch/tcg/insn_trans/trans_bit.c.inc
> @@ -18,13 +18,17 @@ static bool gen_rr(DisasContext *ctx, arg_rr *a,
>
> static void gen_bytepick_w(TCGv dest, TCGv src1, TCGv src2, target_long sa)
> {
> +#ifdef TARGET_LOONGARCH64
> tcg_gen_concat_tl_i64(dest, src1, src2);
> tcg_gen_sextract_i64(dest, dest, (32 - sa * 8), 32);
> +#else
> + tcg_gen_extract2_tl(dest, src1, src2, (32 - sa * 8));
This is the kind of case where using _i32 explicitly emphasizes that the specific size is
required for correctness. You've already got the ifdef to protect the code anyway.
> static void gen_bytepick_d(TCGv dest, TCGv src1, TCGv src2, target_long sa)
> {
> - tcg_gen_extract2_i64(dest, src1, src2, (64 - sa * 8));
> + tcg_gen_extract2_tl(dest, src1, src2, (64 - sa * 8));
> }
>
> static bool gen_bstrins(DisasContext *ctx, arg_rr_ms_ls *a,
> @@ -85,7 +89,7 @@ static void gen_cto_w(TCGv dest, TCGv src1)
>
> static void gen_clz_d(TCGv dest, TCGv src1)
> {
> - tcg_gen_clzi_i64(dest, src1, TARGET_LONG_BITS);
> + tcg_gen_clzi_tl(dest, src1, TARGET_LONG_BITS);
> }
>
> static void gen_clo_d(TCGv dest, TCGv src1)
> @@ -107,8 +111,8 @@ static void gen_cto_d(TCGv dest, TCGv src1)
>
> static void gen_revb_2w(TCGv dest, TCGv src1)
> {
> - tcg_gen_bswap64_i64(dest, src1);
> - tcg_gen_rotri_i64(dest, dest, 32);
> + tcg_gen_bswap_tl(dest, src1);
> + tcg_gen_rotri_tl(dest, dest, 32);
> }
>
> static void gen_revb_2h(TCGv dest, TCGv src1)
> @@ -126,7 +130,7 @@ static void gen_revb_2h(TCGv dest, TCGv src1)
>
> static void gen_revb_4h(TCGv dest, TCGv src1)
> {
> - TCGv mask = tcg_constant_tl(0x00FF00FF00FF00FFULL);
> + TCGv mask = tcg_constant_tl((target_ulong)0x00FF00FF00FF00FFULL);
> TCGv t0 = tcg_temp_new();
> TCGv t1 = tcg_temp_new();
>
> @@ -139,22 +143,22 @@ static void gen_revb_4h(TCGv dest, TCGv src1)
>
> static void gen_revh_2w(TCGv dest, TCGv src1)
> {
> - TCGv_i64 t0 = tcg_temp_new_i64();
> - TCGv_i64 t1 = tcg_temp_new_i64();
> - TCGv_i64 mask = tcg_constant_i64(0x0000ffff0000ffffull);
> + TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> + TCGv mask = tcg_constant_tl((target_ulong)0x0000ffff0000ffffull);
>
> - tcg_gen_shri_i64(t0, src1, 16);
> - tcg_gen_and_i64(t1, src1, mask);
> - tcg_gen_and_i64(t0, t0, mask);
> - tcg_gen_shli_i64(t1, t1, 16);
> - tcg_gen_or_i64(dest, t1, t0);
> + tcg_gen_shri_tl(t0, src1, 16);
> + tcg_gen_and_tl(t1, src1, mask);
> + tcg_gen_and_tl(t0, t0, mask);
> + tcg_gen_shli_tl(t1, t1, 16);
> + tcg_gen_or_tl(dest, t1, t0);
> }
>
> static void gen_revh_d(TCGv dest, TCGv src1)
> {
> TCGv t0 = tcg_temp_new();
> TCGv t1 = tcg_temp_new();
> - TCGv mask = tcg_constant_tl(0x0000FFFF0000FFFFULL);
> + TCGv mask = tcg_constant_tl((target_ulong)0x0000FFFF0000FFFFULL);
>
> tcg_gen_shri_tl(t1, src1, 16);
> tcg_gen_and_tl(t1, t1, mask);
While this allows the code to compile, (1) the functions are unused and (2) they do not
compute the required results. For me, the latter is concerning.
I'd suggest moving GEN_FALSE_TRANS out of trans_privileged.c.inc, then
#ifdef TARGET_LOONGARCH64
// all gen_foo_d
TRANS(bytepick_d, 64, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
// etc
#else
GEN_FALSE_TRANS(bytepick_d)
// etc
#endif
r~
next prev parent reply other threads:[~2024-12-26 21:56 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-26 21:19 [PATCH v2 00/23] target/loongarch: LoongArch32 fixes 1 Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 01/23] target/loongarch: Enable rotr.w/rotri.w for LoongArch32 Jiaxun Yang
2024-12-26 21:24 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 02/23] target/loongarch: Fix address generation for gen_sc Jiaxun Yang
2024-12-26 21:25 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 03/23] target/loongarch: Fix PGD CSR for LoongArch32 Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 04/23] target/loongarch: Perform sign extension for IOCSR reads Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 05/23] target/loongarch: Use target_ulong for iocsrrd helper results Jiaxun Yang
2024-12-26 21:27 ` Richard Henderson
2024-12-26 22:45 ` Philippe Mathieu-Daudé
2024-12-26 21:19 ` [PATCH v2 06/23] target/loongarch: Store some uint64_t values as target_ulong Jiaxun Yang
2024-12-26 22:48 ` Philippe Mathieu-Daudé
2024-12-26 22:58 ` Jiaxun Yang
2024-12-26 23:02 ` Philippe Mathieu-Daudé
2024-12-26 21:19 ` [PATCH v2 07/23] target/loongarch: Cast address to 64bit before DMW_64_VSEG shift Jiaxun Yang
2024-12-26 21:28 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 08/23] target/loongarch: Fix some modifiers for log formatting Jiaxun Yang
2024-12-26 21:29 ` Richard Henderson
2024-12-26 22:49 ` Philippe Mathieu-Daudé
2024-12-26 21:19 ` [PATCH v2 09/23] target/loongarch: Use target_ulong for CSR helpers Jiaxun Yang
2024-12-26 21:31 ` Richard Henderson
2024-12-26 21:39 ` Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 10/23] target/loongarch: Scrutinise TCG float translation for 32 bit build Jiaxun Yang
2024-12-26 22:11 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 11/23] target/loongarch: Scrutinise TCG vector " Jiaxun Yang
2024-12-26 22:25 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 12/23] target/loongarch: Scrutinise TCG bitops " Jiaxun Yang
2024-12-26 21:55 ` Richard Henderson [this message]
2024-12-26 22:08 ` Jiaxun Yang
2024-12-26 22:10 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 13/23] target/loongarch: Fix rdtimer on 32bit build Jiaxun Yang
2024-12-26 22:16 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 14/23] target/loongarch: Scrutinise TCG arithmetic translation for 32 bit build Jiaxun Yang
2024-12-26 22:05 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 15/23] target/loongarch: Fix load type for gen_ll Jiaxun Yang
2024-12-26 22:05 ` Richard Henderson
2024-12-26 21:19 ` [PATCH v2 16/23] target/loongarch: Define address space information for LoongArch32 Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 17/23] target/loongarch: Refactoring is_la64/is_va32 " Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 18/23] target/loongarch: ifdef out 64 bit CPUs on 32 bit builds Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 19/23] target/loongarch: Introduce max32 CPU type Jiaxun Yang
2024-12-26 22:55 ` Philippe Mathieu-Daudé
2024-12-26 23:00 ` Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 20/23] hw/loongarch/virt: Default to max32 CPU for LoongArch 32 build Jiaxun Yang
2024-12-26 22:56 ` Philippe Mathieu-Daudé
2024-12-26 23:03 ` Jiaxun Yang
2024-12-26 23:20 ` Philippe Mathieu-Daudé
2024-12-26 21:19 ` [PATCH v2 21/23] qapi/machine: Replace TARGET_LOONGARCH64 with TARGET_LOONGARCH Jiaxun Yang
2025-01-07 11:06 ` Markus Armbruster
2024-12-26 21:19 ` [PATCH v2 22/23] target/loongarch: Wire up LoongArch32 Kconfigs Jiaxun Yang
2024-12-26 21:19 ` [PATCH v2 23/23] config: Add loongarch32-softmmu target Jiaxun Yang
2024-12-26 22:58 ` Philippe Mathieu-Daudé
2024-12-27 5:20 ` Richard Henderson
2024-12-27 10:48 ` Jiaxun Yang
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=f2360c0b-979a-4473-a7b1-96caa54cff27@linaro.org \
--to=richard.henderson@linaro.org \
--cc=armbru@redhat.com \
--cc=eblake@redhat.com \
--cc=eduardo@habkost.net \
--cc=gaosong@loongson.cn \
--cc=jiaxun.yang@flygoat.com \
--cc=maobibo@loongson.cn \
--cc=marcel.apfelbaum@gmail.com \
--cc=pbonzini@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=wangyanan55@huawei.com \
--cc=zhao1.liu@intel.com \
/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).