From: "Christoph Müllner" <christoph.muellner@vrull.eu>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-riscv@nongnu.org, qemu-devel@nongnu.org,
"Alistair Francis" <alistair.francis@wdc.com>,
"Bin Meng" <bin.meng@windriver.com>,
"Philipp Tomsich" <philipp.tomsich@vrull.eu>,
"Heiko Stübner" <heiko.stuebner@vrull.eu>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
"Nelson Chu" <nelson@rivosinc.com>,
"Kito Cheng" <kito.cheng@sifive.com>,
"Cooper Qu" <cooper.qu@linux.alibaba.com>,
"Lifang Xia" <lifang_xia@linux.alibaba.com>,
"Yunhai Shang" <yunhai@linux.alibaba.com>,
"Zhiwei Liu" <zhiwei_liu@linux.alibaba.com>
Subject: Re: [PATCH v3 09/14] RISC-V: Adding T-Head MemIdx extension
Date: Tue, 31 Jan 2023 19:01:51 +0100 [thread overview]
Message-ID: <CAEg0e7gJeimurDiuFTWqx4OggJ19zD0-c6xBwZkL3sgFWWWeVQ@mail.gmail.com> (raw)
In-Reply-To: <3d864ae7-5430-8db9-f91c-fd24f428b04d@linaro.org>
On Tue, Jan 24, 2023 at 10:21 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/24/23 09:59, Christoph Muellner wrote:
> > +/* XTheadMemIdx */
> > +
> > +/*
> > + * Load with memop from indexed address and add (imm5 << imm2) to rs1.
> > + * If !preinc, then the load address is rs1.
> > + * If preinc, then the load address is rs1 + (imm5) << imm2).
> > + */
> > +static bool gen_load_inc(DisasContext *ctx, arg_th_meminc *a, MemOp memop,
> > + bool preinc)
> > +{
> > + TCGv rd = dest_gpr(ctx, a->rd);
> > + TCGv addr = get_address(ctx, a->rs1, preinc ? a->imm5 << a->imm2 : 0);
> > +
> > + tcg_gen_qemu_ld_tl(rd, addr, ctx->mem_idx, memop);
> > + addr = get_address(ctx, a->rs1, !preinc ? a->imm5 << a->imm2 : 0);
>
> First, you're leaking the previous 'addr' temporary.
Indeed!
The real question is of course, why we call get_address() twice...
Fixed in v4.
> Second, get_address may make modifications to 'addr' which you don't want to write back.
Fixed in v4.
> Third, you are not checking for rd != rs1.
Fixed in v4.
>
> I think what you want is
>
> int imm = a->imm5 << a->imm2;
> TCGv addr = get_address(ctx, a->rs1, preinc ? imm : 0);
> TCGv rd = dest_gpr(ctx, a->rd);
> TCGv rs1 = get_gpr(ctx, a->rs1, EXT_NONE);
>
> tcg_gen_qemu_ld_tl(rd, addr, ctx->mem_idx, memop);
> tcg_gen_addi_tl(rs1, rs1, imm);
> gen_set_gpr(ctx, a->rd, rd);
> gen_set_gpr(ctx, a->rs1, rs1);
Yes (plus the check for rd != rs1).
Fixed in v4.
Thank you!
>
>
> r~
next prev parent reply other threads:[~2023-01-31 18:07 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-24 19:59 [PATCH v3 00/14] Add support for the T-Head vendor extensions Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 01/14] RISC-V: Adding XTheadCmo ISA extension Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 02/14] RISC-V: Adding XTheadSync " Christoph Muellner
2023-01-24 20:21 ` Richard Henderson
2023-01-30 14:05 ` Christoph Müllner
2023-01-24 19:59 ` [PATCH v3 03/14] RISC-V: Adding XTheadBa " Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 04/14] RISC-V: Adding XTheadBb " Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 05/14] RISC-V: Adding XTheadBs " Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 06/14] RISC-V: Adding XTheadCondMov " Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 07/14] RISC-V: Adding T-Head multiply-accumulate instructions Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 08/14] RISC-V: Adding T-Head MemPair extension Christoph Muellner
2023-01-24 20:44 ` Richard Henderson
2023-01-30 2:03 ` LIU Zhiwei
2023-01-30 5:43 ` Richard Henderson
2023-01-30 8:41 ` LIU Zhiwei
2023-01-30 19:03 ` Richard Henderson
2023-01-31 2:34 ` LIU Zhiwei
2023-01-31 18:01 ` Christoph Müllner
2023-01-24 19:59 ` [PATCH v3 09/14] RISC-V: Adding T-Head MemIdx extension Christoph Muellner
2023-01-24 21:21 ` Richard Henderson
2023-01-30 9:04 ` LIU Zhiwei
2023-01-31 18:01 ` Christoph Müllner [this message]
2023-01-24 19:59 ` [PATCH v3 10/14] RISC-V: Adding T-Head FMemIdx extension Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 11/14] RISC-V: Set minimum priv version for Zfh to 1.11 Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 12/14] RISC-V: Add initial support for T-Head C906 Christoph Muellner
2023-01-24 21:26 ` Richard Henderson
2023-01-30 9:06 ` LIU Zhiwei
2023-01-31 18:01 ` Christoph Müllner
2023-01-24 19:59 ` [PATCH v3 13/14] RISC-V: Adding XTheadFmv ISA extension Christoph Muellner
2023-01-24 19:59 ` [PATCH v3 14/14] target/riscv: add a MAINTAINERS entry for XThead* extension support Christoph Muellner
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=CAEg0e7gJeimurDiuFTWqx4OggJ19zD0-c6xBwZkL3sgFWWWeVQ@mail.gmail.com \
--to=christoph.muellner@vrull.eu \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=cooper.qu@linux.alibaba.com \
--cc=heiko.stuebner@vrull.eu \
--cc=kito.cheng@sifive.com \
--cc=lifang_xia@linux.alibaba.com \
--cc=nelson@rivosinc.com \
--cc=palmer@dabbelt.com \
--cc=philipp.tomsich@vrull.eu \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=yunhai@linux.alibaba.com \
--cc=zhiwei_liu@linux.alibaba.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).