From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36126) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTdqa-00024Q-CC for qemu-devel@nongnu.org; Wed, 31 Oct 2012 15:21:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTdqV-0006Rh-M9 for qemu-devel@nongnu.org; Wed, 31 Oct 2012 15:21:04 -0400 Received: from hall.aurel32.net ([88.191.126.93]:49931) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTdqV-0006R3-Ct for qemu-devel@nongnu.org; Wed, 31 Oct 2012 15:20:59 -0400 Date: Wed, 31 Oct 2012 20:20:49 +0100 From: Aurelien Jarno Message-ID: <20121031192049.GA7669@ohm.aurel32.net> References: <874D219413C17C42B1D2E0432B92BE5CBBE221FF@exchdb03.mips.com> <874D219413C17C42B1D2E0432B92BE5CBBE22307@exchdb03.mips.com> <5090B693.4080604@twiddle.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v12 09/14] target-mips: Add ASE DSP bit/manipulation instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jia Liu Cc: Peter Maydell , "Jovanovic, Petar" , "qemu-devel@nongnu.org" , Richard Henderson On Wed, Oct 31, 2012 at 09:29:30PM +0800, Jia Liu wrote: > Hi Richard Peter Jovanovic and Aurelien, > > On Wed, Oct 31, 2012 at 1:26 PM, Richard Henderson wrote: > > On 2012-10-31 01:44, Peter Maydell wrote: > >> On 30 October 2012 15:34, Jia Liu wrote: > >>> On Mon, Oct 29, 2012 at 9:40 PM, Jovanovic, Petar wrote: > >>>>> imm = (int16_t)(imm << 6) >> 6; > >>>> > >>>> result of a bitwise shift of a signed type and a negative vlaue is > >>>> implementation-defined, so you can not rely on that. > >>>> > >>> > >>> I think it will take a 10bits signed value sign extend into 16bits > >>> signed value, and I've tested it with negative values, it working > >>> well. > >> > >> You cannot rely on the behaviour of a specific compiler implementation > >> as evidence that a piece of code is correct. C has a standard which > >> defines what is and is not valid. > > > > Indeed. The only portable way is > > > > val = ((val & (sign | (sign - 1))) ^ sign) - sign > > > > with all unsigned types, and "sign" set to the sign bit. > > > > Thank you very much for the code. > > Is this time OK? As said by Peter, I don't think the code should be modified, the previous code was fine. > static void gen_mipsdsp_bitinsn(CPUMIPSState *env, DisasContext *ctx, > uint32_t op1, uint32_t op2, > int ret, int val) > { > const char *opn = "mipsdsp Bit/ Manipulation"; > TCGv t0; > TCGv val_t; > > if (ret == 0) { > /* Treat as NOP. */ > MIPS_DEBUG("NOP"); > return; > } > > t0 = tcg_temp_new(); > val_t = tcg_temp_new(); > gen_load_gpr(val_t, val); > > #define SIGN_EXTEND10_16(val, sign) \ > val = (((val & (sign | (sign - 1))) ^ sign) - sign) > > switch (op1) { > case OPC_ABSQ_S_PH_DSP: > switch (op2) { > case OPC_BITREV: > check_dsp(ctx); > gen_helper_bitrev(cpu_gpr[ret], val_t); > break; > case OPC_REPL_QB: > check_dsp(ctx); > { > uint32_t imm; > target_long result; > > imm = (ctx->opcode >> 16) & 0xFF; > result = imm << 24 | imm << 16 | imm << 8 | imm; > result = (int32_t)result; > tcg_gen_movi_tl(cpu_gpr[ret], result); > } > break; > case OPC_REPLV_QB: > check_dsp(ctx); > tcg_gen_ext8u_tl(cpu_gpr[ret], val_t); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 8); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 16); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > tcg_gen_ext32s_tl(cpu_gpr[ret], cpu_gpr[ret]); > break; > case OPC_REPL_PH: > check_dsp(ctx); > { > uint16_t imm; > imm = (ctx->opcode >> 16) & 0x03FF; > SIGN_EXTEND10_16(imm, 0x200); > tcg_gen_movi_tl(cpu_gpr[ret], > (target_long)(int32_t) > ((uint32_t)imm << 16 | imm)); > } > break; > case OPC_REPLV_PH: > check_dsp(ctx); > tcg_gen_ext16u_tl(cpu_gpr[ret], val_t); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 16); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > tcg_gen_ext32s_tl(cpu_gpr[ret], cpu_gpr[ret]); > break; > } > break; > #ifdef TARGET_MIPS64 > case OPC_ABSQ_S_QH_DSP: > switch (op2) { > case OPC_REPL_OB: > check_dsp(ctx); > { > target_ulong imm; > > imm = (ctx->opcode >> 16) & 0xFF; > imm = (imm << 8) | imm; > imm = (imm << 16) | imm; > imm = (imm << 32) | imm; > tcg_gen_movi_tl(cpu_gpr[ret], imm); > break; > } > case OPC_REPL_PW: > check_dsp(ctx); > { > target_long imm; > > imm = (ctx->opcode >> 16) & 0x03FF; > SIGN_EXTEND10_16(imm, 0x200); > tcg_gen_movi_tl(cpu_gpr[ret], > (imm << 32) | (imm & 0xFFFFFFFF)); > break; > } > case OPC_REPL_QH: > check_dsp(ctx); > { > target_ulong imm; > > imm = (ctx->opcode >> 16) & 0x03FF; > SIGN_EXTEND10_16(imm, 0x200); > > imm = imm & 0xFFFF; > imm = (imm << 48) | (imm << 32) | (imm << 16) | imm; > tcg_gen_movi_tl(cpu_gpr[ret], imm); > break; > } > case OPC_REPLV_OB: > check_dsp(ctx); > tcg_gen_ext8u_tl(cpu_gpr[ret], val_t); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 8); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 16); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 32); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > break; > case OPC_REPLV_PW: > check_dsp(ctx); > tcg_gen_ext32u_i64(cpu_gpr[ret], val_t); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 32); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > break; > case OPC_REPLV_QH: > check_dsp(ctx); > tcg_gen_ext16u_tl(cpu_gpr[ret], val_t); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 16); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > tcg_gen_shli_tl(t0, cpu_gpr[ret], 32); > tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0); > break; > } > break; > #endif > } > > #undef SIGN_EXTEND10_16 > tcg_temp_free(t0); > tcg_temp_free(val_t); > > (void)opn; /* avoid a compiler warning */ > MIPS_DEBUG("%s", opn); > } > > > >> > >> Having said that, right shift of negative signed integers is one of > >> those bits of implementation defined behaviour which we allow ourselves > >> to rely on in QEMU because all the platforms we care about behave > >> that way. (That integers are 2s complement representation is another.) > > > > Also very true. I don't like seeing the code in question though. > > We've several implementations of sign-extend-to-N-bits functions > > throughout qemu; we ought to unify them. > > > > > > r~ > > Regards, > Jia. > > -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net