From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55174) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXVJT-0001Lq-AE for qemu-devel@nongnu.org; Thu, 03 Sep 2015 10:16:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXVJP-0007rU-3Q for qemu-devel@nongnu.org; Thu, 03 Sep 2015 10:16:27 -0400 Received: from mail.uni-paderborn.de ([131.234.142.9]:54757) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXVJO-0007rI-Qn for qemu-devel@nongnu.org; Thu, 03 Sep 2015 10:16:23 -0400 References: <1441239463-18981-1-git-send-email-rth@twiddle.net> <1441239463-18981-3-git-send-email-rth@twiddle.net> From: Bastian Koppelmann Message-ID: <55E85633.3070909@mail.uni-paderborn.de> Date: Thu, 3 Sep 2015 16:16:19 +0200 MIME-Version: 1.0 In-Reply-To: <1441239463-18981-3-git-send-email-rth@twiddle.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/17] target-openrisc: Streamline arithmetic and OVE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, proljc@gmail.com On 09/03/2015 02:17 AM, Richard Henderson wrote: > + > +void HELPER(ove)(CPUOpenRISCState *env, target_ulong test) > +{ > + if (unlikely(test) && (env->sr & SR_OVE)) { > + OpenRISCCPU *cpu = openrisc_env_get_cpu(env); > + CPUState *cs = CPU(cpu); > + > + cs->exception_index = EXCP_RANGE; > + cpu_restore_state(cs, GETPC()); > + cpu_loop_exit(cs); > + } > +} You forgot to check the AECR register, whether the exception really occurs. > > +static void gen_ove_cy(DisasContext *dc, TCGv cy) > +{ > + gen_helper_ove(cpu_env, cy); > +} > + > +static void gen_ove_ov(DisasContext *dc, TCGv ov) > +{ > + gen_helper_ove(cpu_env, ov); > +} > + Do we really need two functions here? They differ only by the name of the second argument. We should directly use gen_helper_ove (). > +static void gen_add(DisasContext *dc, TCGv dest, TCGv srca, TCGv srcb) > +{ > + TCGv t0 = tcg_const_tl(0); > + TCGv res = tcg_temp_new(); > + TCGv sr_cy = tcg_temp_new(); > + TCGv sr_ov = tcg_temp_new(); > + > + tcg_gen_add2_tl(res, sr_cy, srca, t0, srcb, t0); > + tcg_gen_xor_tl(sr_ov, srca, srcb); > + tcg_gen_xor_tl(t0, res, srcb); > + tcg_gen_andc_tl(sr_ov, t0, sr_ov); > + tcg_temp_free(t0); > + > + tcg_gen_mov_tl(dest, res); > + tcg_temp_free(res); > + > + tcg_gen_shri_tl(sr_ov, sr_ov, TARGET_LONG_BITS - 1); > + tcg_gen_deposit_tl(cpu_sr, cpu_sr, sr_cy, ctz32(SR_CY), 1); > + tcg_gen_deposit_tl(cpu_sr, cpu_sr, sr_ov, ctz32(SR_OV), 1); > + > + gen_ove_cyov(dc, sr_ov, sr_cy); > + tcg_temp_free(sr_ov); > + tcg_temp_free(sr_cy); > +} I do wonder, if we should use TCG globals for sr_cy and sr_ov, as you recommended for the TriCore target. It would not change the helper in case of no ovf/carry, but simplify addc. And we would not need two deposits for every add/sub. > - if (ra != 0 && rb != 0) { > - gen_helper_mul32(cpu_R[rd], cpu_env, cpu_R[ra], cpu_R[rb]); > - } else { > - tcg_gen_movi_tl(cpu_R[rd], 0x0); > - } > + gen_mul(dc, cpu_R[rd], cpu_R[ra], cpu_R[rb]); What happened to this special case here? r0 should always hold zero, so we can keep this optimization here, or at least move it to gen_mul(). > - if (rb != 0 && ra != 0) { > - TCGv_i64 result = tcg_temp_local_new_i64(); > - TCGv_i64 tra = tcg_temp_local_new_i64(); > - TCGv_i64 trb = tcg_temp_local_new_i64(); > - TCGv_i64 high = tcg_temp_new_i64(); > - TCGv_i32 sr_ove = tcg_temp_local_new_i32(); > - TCGLabel *lab = gen_new_label(); > - /* Calculate each result. */ > - tcg_gen_extu_i32_i64(tra, cpu_R[ra]); > - tcg_gen_extu_i32_i64(trb, cpu_R[rb]); > - tcg_gen_mul_i64(result, tra, trb); > - tcg_temp_free_i64(tra); > - tcg_temp_free_i64(trb); > - tcg_gen_shri_i64(high, result, TARGET_LONG_BITS); > - /* Overflow or not. */ > - tcg_gen_brcondi_i64(TCG_COND_EQ, high, 0x00000000, lab); > - tcg_gen_ori_tl(cpu_sr, cpu_sr, (SR_OV | SR_CY)); > - tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE); > - tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab); > - gen_exception(dc, EXCP_RANGE); > - gen_set_label(lab); > - tcg_temp_free_i64(high); > - tcg_gen_trunc_i64_tl(cpu_R[rd], result); > - tcg_temp_free_i64(result); > - tcg_temp_free_i32(sr_ove); > - } else { > - tcg_gen_movi_tl(cpu_R[rd], 0); > - } > + gen_mulu(dc, cpu_R[rd], cpu_R[ra], cpu_R[rb]); > break; The same here. > - { > - if (I16 == 0) { > - tcg_gen_mov_tl(cpu_R[rd], cpu_R[ra]); > - } else { > - TCGLabel *lab = gen_new_label(); > - TCGv_i64 ta = tcg_temp_new_i64(); > - TCGv_i64 td = tcg_temp_local_new_i64(); > - TCGv_i32 res = tcg_temp_local_new_i32(); > - TCGv_i32 sr_ove = tcg_temp_local_new_i32(); > - tcg_gen_extu_i32_i64(ta, cpu_R[ra]); > - tcg_gen_addi_i64(td, ta, sign_extend(I16, 16)); > - tcg_gen_extrl_i64_i32(res, td); > - tcg_gen_shri_i64(td, td, 32); > - tcg_gen_andi_i64(td, td, 0x3); > - /* Jump to lab when no overflow. */ > - tcg_gen_brcondi_i64(TCG_COND_EQ, td, 0x0, lab); > - tcg_gen_brcondi_i64(TCG_COND_EQ, td, 0x3, lab); > - tcg_gen_ori_i32(cpu_sr, cpu_sr, (SR_OV | SR_CY)); > - tcg_gen_andi_i32(sr_ove, cpu_sr, SR_OVE); > - tcg_gen_brcondi_i32(TCG_COND_NE, sr_ove, SR_OVE, lab); > - gen_exception(dc, EXCP_RANGE); > - gen_set_label(lab); > - tcg_gen_mov_i32(cpu_R[rd], res); > - tcg_temp_free_i64(ta); > - tcg_temp_free_i64(td); > - tcg_temp_free_i32(res); > - tcg_temp_free_i32(sr_ove); > - } > - } > + t0 = tcg_const_tl(I16); > + gen_add(dc, cpu_R[rd], cpu_R[ra], t0); > + tcg_temp_free(t0); > break; Again, why remove the special case? > case 0x29: /* l.andi */ > @@ -963,13 +895,9 @@ static void dec_misc(DisasContext *dc, uint32_t insn) > > case 0x2c: /* l.muli */ > LOG_DIS("l.muli r%d, r%d, %d\n", rd, ra, I16); > - if (ra != 0 && I16 != 0) { > - TCGv_i32 im = tcg_const_i32(I16); > - gen_helper_mul32(cpu_R[rd], cpu_env, cpu_R[ra], im); > - tcg_temp_free_i32(im); > - } else { > - tcg_gen_movi_tl(cpu_R[rd], 0x0); > - } > + t0 = tcg_const_tl(I16); > + gen_mul(dc, cpu_R[rd], cpu_R[ra], t0); > + tcg_temp_free(t0); Again, why remove the special case? Cheers, Bastian