From: Richard Henderson <richard.henderson@linaro.org> To: cupertinomiranda@gmail.com, qemu-devel@nongnu.org Cc: shahab@synopsys.com, linux-snps-arc@lists.infradead.org, claziss@synopsys.com, cmiranda@synopsys.com Subject: Re: [PATCH 05/27] arc: TCG instruction generator and hand-definitions Date: Wed, 7 Apr 2021 09:47:45 -0700 [thread overview] Message-ID: <865edf7e-2d01-d272-411f-19e92671aa15@linaro.org> (raw) In-Reply-To: <20210405143138.17016-6-cupertinomiranda@gmail.com> On 4/5/21 7:31 AM, cupertinomiranda@gmail.com wrote: > +/* > + * Function to add boiler plate code for conditional execution. > + * It will add tcg_gen codes only if there is a condition to > + * be checked (ctx->insn.cc != 0). > + * Remember to pair it with gen_cc_epilogue(ctx) macro. > + */ > +static void gen_cc_prologue(DisasCtxt *ctx) > +{ > + ctx->tmp_reg = tcg_temp_local_new(); > + ctx->label = gen_new_label(); There's no point in creating these when insn.cc is false. > + if (ctx->insn.cc) { > + arc_gen_verifyCCFlag(ctx, ctx->tmp_reg); > + tcg_gen_brcondi_tl(TCG_COND_NE, ctx->tmp_reg, 1, ctx->label); You don't need a local temp for tmp_reg. You can create and free it here. > +/* > + * The finishing counter part of gen_cc_prologue. This is supposed > + * to be put at the end of the function using it. > + */ > +static void gen_cc_epilogue(const DisasCtxt *ctx) > +{ > + if (ctx->insn.cc) { > + gen_set_label(ctx->label); > + } > + tcg_temp_free(ctx->tmp_reg); > +} You do not need to keep it live til here. > +/* > + * Populates a 32-bit vector with repeating SHIMM: > + * vec32=(0000000000u6,0000000000u6) > + * vec32=(sssss12,sssss12) > + * It's crucial that the s12 part of an encoding is in signed > + * integer form while passed along in SHIMM, e.g: > + * s12 = -125 (0xf803) --> 0xfffff803 > + * Do not forget to free the returned TCGv_i32 when done! > + */ > +static TCGv_i32 dup_shimm_to_i32(int16_t shimm) > +{ > + TCGv_i32 vec32 = tcg_temp_new_i32(); > + int32_t val = shimm; > + val = ((val << 16) & 0xffff0000) | (val & 0xffff); > + tcg_gen_movi_i32(vec32, val); > + return vec32; > +} return tcg_constant_i32(dup_const(MO_16, shimm)); > > +static TCGv_i64 dup_limm_to_i64(int32_t limm) > +{ > + TCGv_i64 vec64 = tcg_temp_new_i64(); > + int64_t val = limm; > + val = (val << 32) | (val & 0xffffffff); > + tcg_gen_movi_i64(vec64, val); > + return vec64; > +} return tcg_constant_i64(dup_const(MO_32, limm)); > +static TCGv_i64 quad_shimm_to_i64(int16_t shimm) > +{ > + TCGv_i64 vec64 = tcg_temp_new_i64(); > + int64_t val = shimm; > + val = (val << 48) | ((val << 32) & 0x0000ffff00000000) | > + ((val << 16) & 0x00000000ffff0000) | (val & 0xffff); > + tcg_gen_movi_i64(vec64, val); > + return vec64; > +} return tcg_constant_i64(dup_const(MO_16, shimm)); Note that, tcg_constant_* are hashed and need not be freed (but are silently accepted if you do). However, you cannot assign to them. If you really require a mutable temporary, use tcg_const_*. > +static void gen_vec_op2h(const DisasCtxt *ctx, > + void (*OP)(TCGv, TCGv, TCGv), > + TCGv_i32 dest, > + TCGv_i32 b32, > + TCGv_i32 c32) > +{ > + TCGv_i32 t0, t1; > + > + /* If no real register for result, then this a nop. Bail out! */ > + if (!(ctx->insn.operands[0].type & ARC_OPERAND_IR)) { > + return; > + } > + > + t0 = tcg_temp_new(); > + tcg_gen_mov_i32(t0, b32); > + /* > + * If the last operand is a u6/s12, say 63, there is no "HI" in it. > + * Instead, it must be duplicated to form a pair; e.g.: (63, 63). > + */ > + if (ctx->insn.operands[2].type & ARC_OPERAND_SHIMM) { > + t1 = dup_shimm_to_i32(ctx->insn.operands[2].value); > + } else { > + t1 = tcg_temp_new(); > + tcg_gen_mov_i32(t1, c32); > + } > + > + (*OP)(dest, t0, t1); > + > + tcg_temp_free(t1); > + tcg_temp_free(t0); > +} Why are you copying b32 to a new temp? Do you have callbacks that are not careful about overlap between dest, t0, t1, and assume that you have non-overlapping inputs? If you don't require uniqueness, and use tcg_constant_* above, then you don't need to copy c32 into a new temp either, nor worry about conditionally freeing it. > +static void gen_add16(TCGv_i32 dest, TCGv_i32 t0, TCGv_i32 t1) > +{ > + TCGv_i32 tmp = tcg_temp_new_i32(); > + tcg_gen_xor_i32(tmp, t0, t1); > + tcg_gen_andi_i32(tmp, tmp, 0x8000); > + tcg_gen_andi_i32(t0, t0, ~0x8000); > + tcg_gen_andi_i32(t1, t1, ~0x8000); > + tcg_gen_add_i32(t0, t0, t1); > + tcg_gen_xor_i32(dest, t0, tmp); > + tcg_temp_free_i32(tmp); > +} I guess that answers my question of whether you're relying on new unique temps as inputs: yes. > +static void arc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) > +{ > + bool in_a_delayslot_instruction = false; > + DisasContext *dc = container_of(dcbase, DisasContext, base); > + CPUARCState *env = cpu->env_ptr; > + > + /* TODO (issue #62): these must be removed */ > + dc->zero = tcg_const_local_tl(0); > + dc->one = tcg_const_local_tl(1); > + > + if (env->stat.is_delay_slot_instruction == 1) { > + in_a_delayslot_instruction = true; > + } > + > + dc->cpc = dc->base.pc_next; > + decode_opc(env, dc); > + > + dc->base.pc_next = dc->npc; > + tcg_gen_movi_tl(cpu_npc, dc->npc); > + > + if (in_a_delayslot_instruction == true) { > + dc->base.is_jmp = DISAS_NORETURN; > + > + /* Post execution delayslot logic. */ > + TCGLabel *DEf_not_set_label1 = gen_new_label(); > + tcg_gen_brcondi_tl(TCG_COND_NE, cpu_DEf, 1, DEf_not_set_label1); > + tcg_gen_movi_tl(cpu_DEf, 0); > + gen_goto_tb(dc, 1, cpu_bta); > + gen_set_label(DEf_not_set_label1); > + env->stat.is_delay_slot_instruction = 0; You may not examine or modify env directly during translate. This absolutely will not work. r~ _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
WARNING: multiple messages have this Message-ID (diff)
From: Richard Henderson <richard.henderson@linaro.org> To: cupertinomiranda@gmail.com, qemu-devel@nongnu.org Cc: shahab@synopsys.com, linux-snps-arc@lists.infradead.org, claziss@synopsys.com, cmiranda@synopsys.com Subject: Re: [PATCH 05/27] arc: TCG instruction generator and hand-definitions Date: Wed, 7 Apr 2021 09:47:45 -0700 [thread overview] Message-ID: <865edf7e-2d01-d272-411f-19e92671aa15@linaro.org> (raw) In-Reply-To: <20210405143138.17016-6-cupertinomiranda@gmail.com> On 4/5/21 7:31 AM, cupertinomiranda@gmail.com wrote: > +/* > + * Function to add boiler plate code for conditional execution. > + * It will add tcg_gen codes only if there is a condition to > + * be checked (ctx->insn.cc != 0). > + * Remember to pair it with gen_cc_epilogue(ctx) macro. > + */ > +static void gen_cc_prologue(DisasCtxt *ctx) > +{ > + ctx->tmp_reg = tcg_temp_local_new(); > + ctx->label = gen_new_label(); There's no point in creating these when insn.cc is false. > + if (ctx->insn.cc) { > + arc_gen_verifyCCFlag(ctx, ctx->tmp_reg); > + tcg_gen_brcondi_tl(TCG_COND_NE, ctx->tmp_reg, 1, ctx->label); You don't need a local temp for tmp_reg. You can create and free it here. > +/* > + * The finishing counter part of gen_cc_prologue. This is supposed > + * to be put at the end of the function using it. > + */ > +static void gen_cc_epilogue(const DisasCtxt *ctx) > +{ > + if (ctx->insn.cc) { > + gen_set_label(ctx->label); > + } > + tcg_temp_free(ctx->tmp_reg); > +} You do not need to keep it live til here. > +/* > + * Populates a 32-bit vector with repeating SHIMM: > + * vec32=(0000000000u6,0000000000u6) > + * vec32=(sssss12,sssss12) > + * It's crucial that the s12 part of an encoding is in signed > + * integer form while passed along in SHIMM, e.g: > + * s12 = -125 (0xf803) --> 0xfffff803 > + * Do not forget to free the returned TCGv_i32 when done! > + */ > +static TCGv_i32 dup_shimm_to_i32(int16_t shimm) > +{ > + TCGv_i32 vec32 = tcg_temp_new_i32(); > + int32_t val = shimm; > + val = ((val << 16) & 0xffff0000) | (val & 0xffff); > + tcg_gen_movi_i32(vec32, val); > + return vec32; > +} return tcg_constant_i32(dup_const(MO_16, shimm)); > > +static TCGv_i64 dup_limm_to_i64(int32_t limm) > +{ > + TCGv_i64 vec64 = tcg_temp_new_i64(); > + int64_t val = limm; > + val = (val << 32) | (val & 0xffffffff); > + tcg_gen_movi_i64(vec64, val); > + return vec64; > +} return tcg_constant_i64(dup_const(MO_32, limm)); > +static TCGv_i64 quad_shimm_to_i64(int16_t shimm) > +{ > + TCGv_i64 vec64 = tcg_temp_new_i64(); > + int64_t val = shimm; > + val = (val << 48) | ((val << 32) & 0x0000ffff00000000) | > + ((val << 16) & 0x00000000ffff0000) | (val & 0xffff); > + tcg_gen_movi_i64(vec64, val); > + return vec64; > +} return tcg_constant_i64(dup_const(MO_16, shimm)); Note that, tcg_constant_* are hashed and need not be freed (but are silently accepted if you do). However, you cannot assign to them. If you really require a mutable temporary, use tcg_const_*. > +static void gen_vec_op2h(const DisasCtxt *ctx, > + void (*OP)(TCGv, TCGv, TCGv), > + TCGv_i32 dest, > + TCGv_i32 b32, > + TCGv_i32 c32) > +{ > + TCGv_i32 t0, t1; > + > + /* If no real register for result, then this a nop. Bail out! */ > + if (!(ctx->insn.operands[0].type & ARC_OPERAND_IR)) { > + return; > + } > + > + t0 = tcg_temp_new(); > + tcg_gen_mov_i32(t0, b32); > + /* > + * If the last operand is a u6/s12, say 63, there is no "HI" in it. > + * Instead, it must be duplicated to form a pair; e.g.: (63, 63). > + */ > + if (ctx->insn.operands[2].type & ARC_OPERAND_SHIMM) { > + t1 = dup_shimm_to_i32(ctx->insn.operands[2].value); > + } else { > + t1 = tcg_temp_new(); > + tcg_gen_mov_i32(t1, c32); > + } > + > + (*OP)(dest, t0, t1); > + > + tcg_temp_free(t1); > + tcg_temp_free(t0); > +} Why are you copying b32 to a new temp? Do you have callbacks that are not careful about overlap between dest, t0, t1, and assume that you have non-overlapping inputs? If you don't require uniqueness, and use tcg_constant_* above, then you don't need to copy c32 into a new temp either, nor worry about conditionally freeing it. > +static void gen_add16(TCGv_i32 dest, TCGv_i32 t0, TCGv_i32 t1) > +{ > + TCGv_i32 tmp = tcg_temp_new_i32(); > + tcg_gen_xor_i32(tmp, t0, t1); > + tcg_gen_andi_i32(tmp, tmp, 0x8000); > + tcg_gen_andi_i32(t0, t0, ~0x8000); > + tcg_gen_andi_i32(t1, t1, ~0x8000); > + tcg_gen_add_i32(t0, t0, t1); > + tcg_gen_xor_i32(dest, t0, tmp); > + tcg_temp_free_i32(tmp); > +} I guess that answers my question of whether you're relying on new unique temps as inputs: yes. > +static void arc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) > +{ > + bool in_a_delayslot_instruction = false; > + DisasContext *dc = container_of(dcbase, DisasContext, base); > + CPUARCState *env = cpu->env_ptr; > + > + /* TODO (issue #62): these must be removed */ > + dc->zero = tcg_const_local_tl(0); > + dc->one = tcg_const_local_tl(1); > + > + if (env->stat.is_delay_slot_instruction == 1) { > + in_a_delayslot_instruction = true; > + } > + > + dc->cpc = dc->base.pc_next; > + decode_opc(env, dc); > + > + dc->base.pc_next = dc->npc; > + tcg_gen_movi_tl(cpu_npc, dc->npc); > + > + if (in_a_delayslot_instruction == true) { > + dc->base.is_jmp = DISAS_NORETURN; > + > + /* Post execution delayslot logic. */ > + TCGLabel *DEf_not_set_label1 = gen_new_label(); > + tcg_gen_brcondi_tl(TCG_COND_NE, cpu_DEf, 1, DEf_not_set_label1); > + tcg_gen_movi_tl(cpu_DEf, 0); > + gen_goto_tb(dc, 1, cpu_bta); > + gen_set_label(DEf_not_set_label1); > + env->stat.is_delay_slot_instruction = 0; You may not examine or modify env directly during translate. This absolutely will not work. r~
next prev parent reply other threads:[~2021-04-07 16:47 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-05 14:31 *** ARC port for review *** cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 01/27] arc: Add initial core cpu files cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 0:47 ` Richard Henderson 2021-04-07 0:47 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 02/27] arc: Decoder code cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 1:25 ` Richard Henderson 2021-04-07 1:25 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 03/27] arc: Opcode definitions table cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 04/27] arc: TCG and decoder glue code and helpers cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 2:37 ` Richard Henderson 2021-04-07 2:37 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 05/27] arc: TCG instruction generator and hand-definitions cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 3:52 ` Richard Henderson 2021-04-07 3:52 ` Richard Henderson 2021-04-07 16:47 ` Richard Henderson [this message] 2021-04-07 16:47 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 06/27] arc: semfunc.c tcg code generator cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 17:14 ` Richard Henderson 2021-04-07 17:14 ` Richard Henderson 2021-04-07 18:33 ` Peter Maydell 2021-04-07 18:33 ` Peter Maydell 2021-04-05 14:31 ` [PATCH 07/27] arc: TCG instruction definitions cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 19:38 ` Richard Henderson 2021-04-07 19:38 ` Richard Henderson 2021-04-08 0:20 ` Richard Henderson 2021-04-08 0:20 ` Richard Henderson 2021-04-12 14:27 ` Cupertino Miranda 2021-04-12 14:27 ` Cupertino Miranda 2021-04-05 14:31 ` [PATCH 08/27] arc: Add BCR and AUX registers implementation cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 09/27] arc: Add IRQ and timer subsystem support cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 10/27] arc: Add memory management unit (MMU) support cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 11/27] arc: Add memory protection unit (MPU) support cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 12/27] arc: Add gdbstub and XML for debugging support cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 13/27] arc: Add Synopsys ARC emulation boards cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 14/27] arc: Add support for ARCv2 cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 20:30 ` Richard Henderson 2021-04-07 20:30 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 15/27] tests/tcg: ARC: Add TCG instruction definition tests cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 20:38 ` Richard Henderson 2021-04-07 20:38 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 16/27] tests/acceptance: ARC: Add linux boot testing cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 20:40 ` Richard Henderson 2021-04-07 20:40 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 17/27] arcv3: Core cpu file changes cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 18/27] arcv3: Decoder code cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 23:07 ` Richard Henderson 2021-04-07 23:07 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 19/27] arcv3: Opcode definition table cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 20/27] arcv3: TCG, decoder glue code and helper changes cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 23:36 ` Richard Henderson 2021-04-07 23:36 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 21/27] arcv3: TCG instruction generator changes cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 23:43 ` Richard Henderson 2021-04-07 23:43 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 22/27] arcv3: TCG instruction definitions cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-07 23:48 ` Richard Henderson 2021-04-07 23:48 ` Richard Henderson 2021-04-05 14:31 ` [PATCH 23/27] arcv3: BCR and AUX register changes cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 24/27] arcv3: IRQ changes and new MMUv6 WIP cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 25/27] arcv3: gdbstub changes and new XML files cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 26/27] arcv3: board changes cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-05 14:31 ` [PATCH 27/27] arcv3: Add support for ARCv3 cupertinomiranda 2021-04-05 14:31 ` cupertinomiranda 2021-04-06 23:47 ` *** ARC port for review *** Richard Henderson 2021-04-06 23:47 ` Richard Henderson 2021-04-12 14:25 ` Cupertino Miranda 2021-04-12 14:25 ` Cupertino Miranda
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=865edf7e-2d01-d272-411f-19e92671aa15@linaro.org \ --to=richard.henderson@linaro.org \ --cc=claziss@synopsys.com \ --cc=cmiranda@synopsys.com \ --cc=cupertinomiranda@gmail.com \ --cc=linux-snps-arc@lists.infradead.org \ --cc=qemu-devel@nongnu.org \ --cc=shahab@synopsys.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: linkBe 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.