From: Brian Cain <bcain@quicinc.com>
To: Taylor Simpson <ltaylorsimpson@gmail.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "Matheus Bernardino (QUIC)" <quic_mathbern@quicinc.com>,
Sid Manning <sidneym@quicinc.com>,
"Marco Liebel (QUIC)" <quic_mliebel@quicinc.com>,
"richard.henderson@linaro.org" <richard.henderson@linaro.org>,
"philmd@linaro.org" <philmd@linaro.org>,
"ale@rev.ng" <ale@rev.ng>, "anjo@rev.ng" <anjo@rev.ng>
Subject: RE: [PATCH v2 3/3] Hexagon (target/hexagon) Enable more short-circuit packets (HVX)
Date: Fri, 29 Mar 2024 02:02:04 +0000 [thread overview]
Message-ID: <CH3PR02MB102475481D029351E5DCA5F66B83A2@CH3PR02MB10247.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20240201103340.119081-4-ltaylorsimpson@gmail.com>
> -----Original Message-----
> From: Taylor Simpson <ltaylorsimpson@gmail.com>
> Sent: Thursday, February 1, 2024 4:34 AM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; Sid Manning <sidneym@quicinc.com>;
> Marco Liebel (QUIC) <quic_mliebel@quicinc.com>;
> richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng; anjo@rev.ng;
> ltaylorsimpson@gmail.com
> Subject: [PATCH v2 3/3] Hexagon (target/hexagon) Enable more short-circuit
> packets (HVX)
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> Look for read-after-write instead of overlap of reads and writes
>
> HVX instructions with helpers have pass-by-reference semantics, so
> we check for overlaps of reads and writes within the same instruction.
>
> Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> ---
> target/hexagon/translate.h | 88 +++++++++++++++++++++++------
> target/hexagon/translate.c | 58 ++-----------------
> target/hexagon/gen_analyze_funcs.py | 19 ++++---
> target/hexagon/hex_common.py | 45 ++++++++++-----
> 4 files changed, 115 insertions(+), 95 deletions(-)
>
> diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h
> index d5e7f49ad8..00cc2bcd63 100644
> --- a/target/hexagon/translate.h
> +++ b/target/hexagon/translate.h
> @@ -50,23 +50,27 @@ typedef struct DisasContext {
> int tmp_vregs_num[VECTOR_TEMPS_MAX];
> int vreg_log[NUM_VREGS];
> int vreg_log_idx;
> + DECLARE_BITMAP(vregs_written, NUM_VREGS);
> + DECLARE_BITMAP(insn_vregs_written, NUM_VREGS);
> DECLARE_BITMAP(vregs_updated_tmp, NUM_VREGS);
> DECLARE_BITMAP(vregs_updated, NUM_VREGS);
> DECLARE_BITMAP(vregs_select, NUM_VREGS);
> DECLARE_BITMAP(predicated_future_vregs, NUM_VREGS);
> DECLARE_BITMAP(predicated_tmp_vregs, NUM_VREGS);
> - DECLARE_BITMAP(vregs_read, NUM_VREGS);
> + DECLARE_BITMAP(insn_vregs_read, NUM_VREGS);
> int qreg_log[NUM_QREGS];
> int qreg_log_idx;
> - DECLARE_BITMAP(qregs_read, NUM_QREGS);
> + DECLARE_BITMAP(qregs_written, NUM_QREGS);
> + DECLARE_BITMAP(insn_qregs_written, NUM_QREGS);
> + DECLARE_BITMAP(insn_qregs_read, NUM_QREGS);
> bool pre_commit;
> bool need_commit;
> TCGCond branch_cond;
> target_ulong branch_dest;
> bool is_tight_loop;
> bool short_circuit;
> - bool has_hvx_helper;
> bool read_after_write;
> + bool has_hvx_overlap;
> TCGv new_value[TOTAL_PER_THREAD_REGS];
> TCGv new_pred_value[NUM_PREGS];
> TCGv pred_written;
> @@ -146,10 +150,25 @@ intptr_t ctx_future_vreg_off(DisasContext *ctx, int
> regnum,
> intptr_t ctx_tmp_vreg_off(DisasContext *ctx, int regnum,
> int num, bool alloc_ok);
>
> +static inline void ctx_start_hvx_insn(DisasContext *ctx)
> +{
> + bitmap_zero(ctx->insn_vregs_written, NUM_VREGS);
> + bitmap_zero(ctx->insn_vregs_read, NUM_VREGS);
> + bitmap_zero(ctx->insn_qregs_written, NUM_QREGS);
> + bitmap_zero(ctx->insn_qregs_read, NUM_QREGS);
> +}
> +
> static inline void ctx_log_vreg_write(DisasContext *ctx,
> int rnum, VRegWriteType type,
> - bool is_predicated)
> + bool is_predicated, bool has_helper)
> {
> + if (has_helper) {
> + set_bit(rnum, ctx->insn_vregs_written);
> + if (test_bit(rnum, ctx->insn_vregs_read)) {
> + ctx->has_hvx_overlap = true;
> + }
> + }
> + set_bit(rnum, ctx->vregs_written);
> if (type != EXT_TMP) {
> if (!test_bit(rnum, ctx->vregs_updated)) {
> ctx->vreg_log[ctx->vreg_log_idx] = rnum;
> @@ -175,42 +194,77 @@ static inline void ctx_log_vreg_write(DisasContext
> *ctx,
>
> static inline void ctx_log_vreg_write_pair(DisasContext *ctx,
> int rnum, VRegWriteType type,
> - bool is_predicated)
> + bool is_predicated, bool has_helper)
> {
> - ctx_log_vreg_write(ctx, rnum ^ 0, type, is_predicated);
> - ctx_log_vreg_write(ctx, rnum ^ 1, type, is_predicated);
> + ctx_log_vreg_write(ctx, rnum ^ 0, type, is_predicated, has_helper);
> + ctx_log_vreg_write(ctx, rnum ^ 1, type, is_predicated, has_helper);
> }
>
> -static inline void ctx_log_vreg_read(DisasContext *ctx, int rnum)
> +static inline void ctx_log_vreg_read(DisasContext *ctx, int rnum,
> + bool has_helper)
> {
> - set_bit(rnum, ctx->vregs_read);
> + if (has_helper) {
> + set_bit(rnum, ctx->insn_vregs_read);
> + if (test_bit(rnum, ctx->insn_vregs_written)) {
> + ctx->has_hvx_overlap = true;
> + }
> + }
> + if (test_bit(rnum, ctx->vregs_written)) {
> + ctx->read_after_write = true;
> + }
> }
>
> -static inline void ctx_log_vreg_read_new(DisasContext *ctx, int rnum)
> +static inline void ctx_log_vreg_read_new(DisasContext *ctx, int rnum,
> + bool has_helper)
> {
> g_assert(is_gather_store_insn(ctx) ||
> test_bit(rnum, ctx->vregs_updated) ||
> test_bit(rnum, ctx->vregs_select) ||
> test_bit(rnum, ctx->vregs_updated_tmp));
> - set_bit(rnum, ctx->vregs_read);
> + if (has_helper) {
> + set_bit(rnum, ctx->insn_vregs_read);
> + if (test_bit(rnum, ctx->insn_vregs_written)) {
> + ctx->has_hvx_overlap = true;
> + }
> + }
> + if (is_gather_store_insn(ctx)) {
> + ctx->read_after_write = true;
> + }
> }
>
> -static inline void ctx_log_vreg_read_pair(DisasContext *ctx, int rnum)
> +static inline void ctx_log_vreg_read_pair(DisasContext *ctx, int rnum,
> + bool has_helper)
> {
> - ctx_log_vreg_read(ctx, rnum ^ 0);
> - ctx_log_vreg_read(ctx, rnum ^ 1);
> + ctx_log_vreg_read(ctx, rnum ^ 0, has_helper);
> + ctx_log_vreg_read(ctx, rnum ^ 1, has_helper);
> }
>
> static inline void ctx_log_qreg_write(DisasContext *ctx,
> - int rnum)
> + int rnum, bool has_helper)
> {
> + if (has_helper) {
> + set_bit(rnum, ctx->insn_qregs_written);
> + if (test_bit(rnum, ctx->insn_qregs_read)) {
> + ctx->has_hvx_overlap = true;
> + }
> + }
> + set_bit(rnum, ctx->qregs_written);
> ctx->qreg_log[ctx->qreg_log_idx] = rnum;
> ctx->qreg_log_idx++;
> }
>
> -static inline void ctx_log_qreg_read(DisasContext *ctx, int qnum)
> +static inline void ctx_log_qreg_read(DisasContext *ctx,
> + int qnum, bool has_helper)
> {
> - set_bit(qnum, ctx->qregs_read);
> + if (has_helper) {
> + set_bit(qnum, ctx->insn_qregs_read);
> + if (test_bit(qnum, ctx->insn_qregs_written)) {
> + ctx->has_hvx_overlap = true;
> + }
> + }
> + if (test_bit(qnum, ctx->qregs_written)) {
> + ctx->read_after_write = true;
> + }
> }
>
> extern TCGv hex_gpr[TOTAL_PER_THREAD_REGS];
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index 751ca71790..ed4b4acd1d 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -378,60 +378,10 @@ static bool need_commit(DisasContext *ctx)
> return true;
> }
>
> - if (pkt->num_insns == 1) {
> - if (pkt->pkt_has_hvx) {
> - /*
> - * The HVX instructions with generated helpers use
> - * pass-by-reference, so they need the read/write overlap
> - * check below.
> - * The HVX instructions with overrides are OK.
> - */
> - if (!ctx->has_hvx_helper) {
> - return false;
> - }
> - } else {
> - return false;
> - }
> - }
> -
> - if (ctx->read_after_write) {
> + if (ctx->read_after_write || ctx->has_hvx_overlap) {
> return true;
> }
>
> - /* Check for overlap between HVX reads and writes */
> - for (int i = 0; i < ctx->vreg_log_idx; i++) {
> - int vnum = ctx->vreg_log[i];
> - if (test_bit(vnum, ctx->vregs_read)) {
> - return true;
> - }
> - }
> - if (!bitmap_empty(ctx->vregs_updated_tmp, NUM_VREGS)) {
> - int i = find_first_bit(ctx->vregs_updated_tmp, NUM_VREGS);
> - while (i < NUM_VREGS) {
> - if (test_bit(i, ctx->vregs_read)) {
> - return true;
> - }
> - i = find_next_bit(ctx->vregs_updated_tmp, NUM_VREGS, i + 1);
> - }
> - }
> - if (!bitmap_empty(ctx->vregs_select, NUM_VREGS)) {
> - int i = find_first_bit(ctx->vregs_select, NUM_VREGS);
> - while (i < NUM_VREGS) {
> - if (test_bit(i, ctx->vregs_read)) {
> - return true;
> - }
> - i = find_next_bit(ctx->vregs_select, NUM_VREGS, i + 1);
> - }
> - }
> -
> - /* Check for overlap between HVX predicate reads and writes */
> - for (int i = 0; i < ctx->qreg_log_idx; i++) {
> - int qnum = ctx->qreg_log[i];
> - if (test_bit(qnum, ctx->qregs_read)) {
> - return true;
> - }
> - }
> -
> return false;
> }
>
> @@ -453,8 +403,8 @@ static void mark_implicit_pred_reads(DisasContext
> *ctx)
> static void analyze_packet(DisasContext *ctx)
> {
> Packet *pkt = ctx->pkt;
> - ctx->has_hvx_helper = false;
> ctx->read_after_write = false;
> + ctx->has_hvx_overlap = false;
> for (int i = 0; i < pkt->num_insns; i++) {
> Insn *insn = &pkt->insn[i];
> ctx->insn = insn;
> @@ -485,13 +435,13 @@ static void gen_start_packet(DisasContext *ctx)
> ctx->future_vregs_idx = 0;
> ctx->tmp_vregs_idx = 0;
> ctx->vreg_log_idx = 0;
> + bitmap_zero(ctx->vregs_written, NUM_VREGS);
> bitmap_zero(ctx->vregs_updated_tmp, NUM_VREGS);
> bitmap_zero(ctx->vregs_updated, NUM_VREGS);
> bitmap_zero(ctx->vregs_select, NUM_VREGS);
> bitmap_zero(ctx->predicated_future_vregs, NUM_VREGS);
> bitmap_zero(ctx->predicated_tmp_vregs, NUM_VREGS);
> - bitmap_zero(ctx->vregs_read, NUM_VREGS);
> - bitmap_zero(ctx->qregs_read, NUM_QREGS);
> + bitmap_zero(ctx->qregs_written, NUM_QREGS);
> ctx->qreg_log_idx = 0;
> for (i = 0; i < STORES_MAX; i++) {
> ctx->store_width[i] = 0;
> diff --git a/target/hexagon/gen_analyze_funcs.py
> b/target/hexagon/gen_analyze_funcs.py
> index 890e6a3a95..81e1d9cfa3 100755
> --- a/target/hexagon/gen_analyze_funcs.py
> +++ b/target/hexagon/gen_analyze_funcs.py
> @@ -43,6 +43,16 @@ def gen_analyze_func(f, tag, regs, imms):
> f.write("{\n")
>
> f.write(" Insn *insn G_GNUC_UNUSED = ctx->insn;\n")
> + if (hex_common.is_hvx_insn(tag)):
> + if hex_common.has_hvx_helper(tag):
> + f.write(
> + " const bool G_GNUC_UNUSED insn_has_hvx_helper = true;\n"
> + )
> + f.write(" ctx_start_hvx_insn(ctx);\n")
> + else:
> + f.write(
> + " const bool G_GNUC_UNUSED insn_has_hvx_helper = false;\n"
> + )
>
> ## Declare all the registers
> for regno, register in enumerate(regs):
> @@ -64,15 +74,6 @@ def gen_analyze_func(f, tag, regs, imms):
> if reg.is_written():
> reg.analyze_write(f, tag, regno)
>
> - has_generated_helper = not hex_common.skip_qemu_helper(
> - tag
> - ) and not hex_common.is_idef_parser_enabled(tag)
> -
> - ## Mark HVX instructions with generated helpers
> - if (has_generated_helper and
> - "A_CVI" in hex_common.attribdict[tag]):
> - f.write(" ctx->has_hvx_helper = true;\n")
> -
> f.write("}\n\n")
>
>
> diff --git a/target/hexagon/hex_common.py
> b/target/hexagon/hex_common.py
> index 33801e4bd7..9e7f613e3c 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -241,6 +241,16 @@ def is_idef_parser_enabled(tag):
> return tag in idef_parser_enabled
>
>
> +def is_hvx_insn(tag):
> + return "A_CVI" in attribdict[tag]
> +
> +
> +def has_hvx_helper(tag):
> + return (is_hvx_insn(tag) and
> + not skip_qemu_helper(tag) and
> + not is_idef_parser_enabled(tag))
> +
> +
> def imm_name(immlett):
> return f"{immlett}iV"
>
> @@ -704,7 +714,8 @@ def analyze_write(self, f, tag, regno):
> newv = hvx_newv(tag)
> predicated = "true" if is_predicated(tag) else "false"
> f.write(code_fmt(f"""\
> - ctx_log_vreg_write(ctx, {self.reg_num}, {newv}, {predicated});
> + ctx_log_vreg_write(ctx, {self.reg_num}, {newv}, {predicated},
> + insn_has_hvx_helper);
> """))
>
> class VRegSource(Register, Hvx, OldSource):
> @@ -724,7 +735,7 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_vreg_read(ctx, {self.reg_num});
> + ctx_log_vreg_read(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
>
> class VRegNewSource(Register, Hvx, NewSource):
> @@ -741,7 +752,7 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_vreg_read_new(ctx, {self.reg_num});
> + ctx_log_vreg_read_new(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
>
> class VRegReadWrite(Register, Hvx, ReadWrite):
> @@ -767,13 +778,14 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_vreg_read(ctx, {self.reg_num});
> + ctx_log_vreg_read(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
> def analyze_write(self, f, tag, regno):
> newv = hvx_newv(tag)
> predicated = "true" if is_predicated(tag) else "false"
> f.write(code_fmt(f"""\
> - ctx_log_vreg_write(ctx, {self.reg_num}, {newv}, {predicated});
> + ctx_log_vreg_write(ctx, {self.reg_num}, {newv}, {predicated},
> + insn_has_hvx_helper);
> """))
>
> class VRegTmp(Register, Hvx, ReadWrite):
> @@ -801,13 +813,14 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_vreg_read(ctx, {self.reg_num});
> + ctx_log_vreg_read(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
> def analyze_write(self, f, tag, regno):
> newv = hvx_newv(tag)
> predicated = "true" if is_predicated(tag) else "false"
> f.write(code_fmt(f"""\
> - ctx_log_vreg_write(ctx, {self.reg_num}, {newv}, {predicated});
> + ctx_log_vreg_write(ctx, {self.reg_num}, {newv}, {predicated},
> + insn_has_hvx_helper);
> """))
>
> class VRegPairDest(Register, Hvx, Dest):
> @@ -832,7 +845,8 @@ def analyze_write(self, f, tag, regno):
> newv = hvx_newv(tag)
> predicated = "true" if is_predicated(tag) else "false"
> f.write(code_fmt(f"""\
> - ctx_log_vreg_write_pair(ctx, {self.reg_num}, {newv}, {predicated});
> + ctx_log_vreg_write_pair(ctx, {self.reg_num}, {newv}, {predicated},
> + insn_has_hvx_helper);
> """))
>
> class VRegPairSource(Register, Hvx, OldSource):
> @@ -859,7 +873,7 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_vreg_read_pair(ctx, {self.reg_num});
> + ctx_log_vreg_read_pair(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
>
> class VRegPairReadWrite(Register, Hvx, ReadWrite):
> @@ -891,13 +905,14 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_vreg_read_pair(ctx, {self.reg_num});
> + ctx_log_vreg_read_pair(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
> def analyze_write(self, f, tag, regno):
> newv = hvx_newv(tag)
> predicated = "true" if is_predicated(tag) else "false"
> f.write(code_fmt(f"""\
> - ctx_log_vreg_write_pair(ctx, {self.reg_num}, {newv}, {predicated});
> + ctx_log_vreg_write_pair(ctx, {self.reg_num}, {newv}, {predicated},
> + insn_has_hvx_helper);
> """))
>
> class QRegDest(Register, Hvx, Dest):
> @@ -920,7 +935,7 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_write(self, f, tag, regno):
> f.write(code_fmt(f"""\
> - ctx_log_qreg_write(ctx, {self.reg_num});
> + ctx_log_qreg_write(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
>
> class QRegSource(Register, Hvx, OldSource):
> @@ -941,7 +956,7 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_qreg_read(ctx, {self.reg_num});
> + ctx_log_qreg_read(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
>
> class QRegReadWrite(Register, Hvx, ReadWrite):
> @@ -967,11 +982,11 @@ def helper_hvx_desc(self, f):
> """))
> def analyze_read(self, f, regno):
> f.write(code_fmt(f"""\
> - ctx_log_qreg_read(ctx, {self.reg_num});
> + ctx_log_qreg_read(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
> def analyze_write(self, f, tag, regno):
> f.write(code_fmt(f"""\
> - ctx_log_qreg_write(ctx, {self.reg_num});
> + ctx_log_qreg_write(ctx, {self.reg_num}, insn_has_hvx_helper);
> """))
>
> def init_registers():
> --
> 2.34.1
Reviewed-by: Brian Cain <bcain@quicinc.com>
prev parent reply other threads:[~2024-03-29 2:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-01 10:33 [PATCH v2 0/3] Hexagon (target/hexagon) Enable more short-circuit packets Taylor Simpson
2024-02-01 10:33 ` [PATCH v2 1/3] Hexagon (target/hexagon) Analyze reads before writes Taylor Simpson
2024-03-29 2:05 ` Brian Cain
2024-02-01 10:33 ` [PATCH v2 2/3] Hexagon (target/hexagon) Enable more short-circuit packets (scalar core) Taylor Simpson
2024-03-29 2:02 ` Brian Cain
2024-02-01 10:33 ` [PATCH v2 3/3] Hexagon (target/hexagon) Enable more short-circuit packets (HVX) Taylor Simpson
2024-03-29 2:02 ` Brian Cain [this message]
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=CH3PR02MB102475481D029351E5DCA5F66B83A2@CH3PR02MB10247.namprd02.prod.outlook.com \
--to=bcain@quicinc.com \
--cc=ale@rev.ng \
--cc=anjo@rev.ng \
--cc=ltaylorsimpson@gmail.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=quic_mathbern@quicinc.com \
--cc=quic_mliebel@quicinc.com \
--cc=richard.henderson@linaro.org \
--cc=sidneym@quicinc.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.