> -----Original Message-----
> From: Taylor Simpson <ltaylorsimp...@gmail.com>
> Sent: Thursday, February 1, 2024 4:34 AM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain <bc...@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathb...@quicinc.com>; Sid Manning <sidn...@quicinc.com>;
> Marco Liebel (QUIC) <quic_mlie...@quicinc.com>;
> richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng; a...@rev.ng;
> ltaylorsimp...@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 <ltaylorsimp...@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 <bc...@quicinc.com>
