From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Huang Tao <eric.huang@linux.alibaba.com>,
qemu-devel@nongnu.org, dbarboza@ventanamicro.com
Cc: qemu-riscv@nongnu.org, zhiwei_liu@linux.alibaba.com,
liwei1518@gmail.com, bin.meng@windriver.com,
alistair.francis@wdc.com, palmer@dabbelt.com,
Christoph Muellner <christoph.muellner@vrull.eu>
Subject: Re: [PATCH v3] target/riscv: Implement dynamic establishment of custom decoder
Date: Thu, 14 Mar 2024 09:18:09 +0100 [thread overview]
Message-ID: <d848a938-f47f-47cc-adff-130a418facbb@linaro.org> (raw)
In-Reply-To: <e5a3cb94-b45d-476a-9d83-04e47ace7c2b@linux.alibaba.com>
On 14/3/24 07:24, Huang Tao wrote:
> Hi,
>
> On 2024/3/13 18:47, Philippe Mathieu-Daudé wrote:
>>
>>> +{
>>> + GPtrArray *dynamic_decoders;
>>> + dynamic_decoders = g_ptr_array_sized_new(decoder_table_size);
>>> + for (size_t i = 0; i < decoder_table_size; ++i) {
>>> + if (decoder_table[i].guard_func &&
>>> + decoder_table[i].guard_func(&cpu->cfg)) {
>>> + g_ptr_array_add(dynamic_decoders,
>>> + (gpointer)decoder_table[i].decode_fn);
>>> + }
>>> + }
>>> +
>>> + cpu->decoders = dynamic_decoders;
>>> +}
>>
>> Move this function to translate.c and make decoder_table[] static.
>> Then we don't need the "cpu_decoder.h", it is specific to TCG and
>> declarations go in "target/riscv/tcg/tcg-cpu.h".
>>
> This function is about finalizing the feature of cpu, it is not suitable
> to move it to translate.c from the perspective of code structure and
> readability.
>
> I will try to move the function to tcg-cpu.c, and the declarations to
> tcg-cpu.h according to your suggestion.
>
>>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>>> index 177418b2b9..332f0bfd4e 100644
>>> --- a/target/riscv/translate.c
>>> +++ b/target/riscv/translate.c
>>> @@ -115,6 +115,7 @@ typedef struct DisasContext {
>>> bool frm_valid;
>>> /* TCG of the current insn_start */
>>> TCGOp *insn_start;
>>> + const GPtrArray *decoders;
>>
>> Why do we need this reference? We can use env_archcpu(env)->decoders.
>>
> As Richard said before:
>
> > We try to avoid placing env into DisasContext, so that it is much
> harder to make the mistake of referencing env fields at
> translation-time, when you really needed to generate tcg code to
> reference the fields at runtime.
Right, he told me the same on IRC recently ;)
>
> It also applies to the ArchCPU case.
>
>
> Thanks to your review, I will adopt the other suggestions in the next
> version.
>
>
> Thanks,
>
> Huang Tao
>
>
>
next prev parent reply other threads:[~2024-03-14 8:18 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-13 9:57 [PATCH v3] target/riscv: Implement dynamic establishment of custom decoder Huang Tao
2024-03-13 10:47 ` Philippe Mathieu-Daudé
2024-03-14 6:24 ` Huang Tao
2024-03-14 8:18 ` Philippe Mathieu-Daudé [this message]
2024-03-13 20:32 ` Richard Henderson
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=d848a938-f47f-47cc-adff-130a418facbb@linaro.org \
--to=philmd@linaro.org \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=christoph.muellner@vrull.eu \
--cc=dbarboza@ventanamicro.com \
--cc=eric.huang@linux.alibaba.com \
--cc=liwei1518@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--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 read-only IMAP folder(s) and NNTP newsgroup(s).