From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47319) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSS1g-0006Ac-0t for qemu-devel@nongnu.org; Fri, 12 Sep 2014 10:40:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XSS1U-0007rX-FM for qemu-devel@nongnu.org; Fri, 12 Sep 2014 10:40:39 -0400 Message-ID: <541305D5.4040904@gmail.com> Date: Fri, 12 Sep 2014 09:40:21 -0500 From: Tom Musta MIME-Version: 1.0 References: <1410325413> <1410463065-4400-1-git-send-email-mallard.pierre@gmail.com> <1410463065-4400-2-git-send-email-mallard.pierre@gmail.com> <54130322.3030006@gmail.com> In-Reply-To: <54130322.3030006@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] target-ppc : Allow fc[tf]id[*] mnemonics for non TARGET_PPC64 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pierre Mallard , qemu-devel@nongnu.org, qemu-ppc@nongnu.org Cc: agraf@suse.de On 9/12/2014 9:28 AM, Tom Musta wrote: > On 9/11/2014 2:17 PM, Pierre Mallard wrote: >> This patch remove limitation for fc[tf]id[*] on 32 bits targets and >> add a new insn flag for signed integer 64 conversion PPC2_FP_CVT_S64 >> --- >> target-ppc/cpu.h | 5 ++++- >> target-ppc/fpu_helper.c | 6 ------ >> target-ppc/helper.h | 4 +--- >> target-ppc/translate.c | 18 +++++++----------- >> target-ppc/translate_init.c | 9 ++++++--- >> 5 files changed, 18 insertions(+), 24 deletions(-) >> >> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h >> index b64c652..fa50c32 100644 >> --- a/target-ppc/cpu.h >> +++ b/target-ppc/cpu.h >> @@ -2008,13 +2008,16 @@ enum { >> PPC2_ALTIVEC_207 = 0x0000000000004000ULL, >> /* PowerISA 2.07 Book3s specification */ >> PPC2_ISA207S = 0x0000000000008000ULL, >> + /* Double precision floating point conversion for signed integer 64 */ >> + PPC2_FP_CVT_S64 = 0x0000000000010000ULL, >> >> #define PPC_TCG_INSNS2 (PPC2_BOOKE206 | PPC2_VSX | PPC2_PRCNTL | PPC2_DBRX | \ >> PPC2_ISA205 | PPC2_VSX207 | PPC2_PERM_ISA206 | \ >> PPC2_DIVE_ISA206 | PPC2_ATOMIC_ISA206 | \ >> PPC2_FP_CVT_ISA206 | PPC2_FP_TST_ISA206 | \ >> PPC2_BCTAR_ISA207 | PPC2_LSQ_ISA207 | \ >> - PPC2_ALTIVEC_207 | PPC2_ISA207S | PPC2_DFP) >> + PPC2_ALTIVEC_207 | PPC2_ISA207S | PPC2_DFP | \ >> + PPC2_FP_CVT_S64) >> }; >> >> /*****************************************************************************/ >> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c >> index da93d12..7f74466 100644 >> --- a/target-ppc/fpu_helper.c >> +++ b/target-ppc/fpu_helper.c >> @@ -649,14 +649,10 @@ FPU_FCTI(fctiw, int32, 0x80000000U) >> FPU_FCTI(fctiwz, int32_round_to_zero, 0x80000000U) >> FPU_FCTI(fctiwu, uint32, 0x00000000U) >> FPU_FCTI(fctiwuz, uint32_round_to_zero, 0x00000000U) >> -#if defined(TARGET_PPC64) >> FPU_FCTI(fctid, int64, 0x8000000000000000ULL) >> FPU_FCTI(fctidz, int64_round_to_zero, 0x8000000000000000ULL) >> FPU_FCTI(fctidu, uint64, 0x0000000000000000ULL) >> FPU_FCTI(fctiduz, uint64_round_to_zero, 0x0000000000000000ULL) >> -#endif >> - >> -#if defined(TARGET_PPC64) >> >> #define FPU_FCFI(op, cvtr, is_single) \ >> uint64_t helper_##op(CPUPPCState *env, uint64_t arg) \ >> @@ -678,8 +674,6 @@ FPU_FCFI(fcfids, int64_to_float32, 1) >> FPU_FCFI(fcfidu, uint64_to_float64, 0) >> FPU_FCFI(fcfidus, uint64_to_float32, 1) >> >> -#endif >> - >> static inline uint64_t do_fri(CPUPPCState *env, uint64_t arg, >> int rounding_mode) >> { >> diff --git a/target-ppc/helper.h b/target-ppc/helper.h >> index 509eae5..52402ef 100644 >> --- a/target-ppc/helper.h >> +++ b/target-ppc/helper.h >> @@ -67,16 +67,14 @@ DEF_HELPER_2(fctiw, i64, env, i64) >> DEF_HELPER_2(fctiwu, i64, env, i64) >> DEF_HELPER_2(fctiwz, i64, env, i64) >> DEF_HELPER_2(fctiwuz, i64, env, i64) >> -#if defined(TARGET_PPC64) >> DEF_HELPER_2(fcfid, i64, env, i64) >> DEF_HELPER_2(fcfidu, i64, env, i64) >> DEF_HELPER_2(fcfids, i64, env, i64) >> DEF_HELPER_2(fcfidus, i64, env, i64) >> DEF_HELPER_2(fctid, i64, env, i64) >> -DEF_HELPER_2(fctidu, i64, env, i64) >> DEF_HELPER_2(fctidz, i64, env, i64) >> +DEF_HELPER_2(fctidu, i64, env, i64) > > NIT: I would not have re-arranged fctidu/fctidz like this since it only makes the patch larger without actually accomplishing anything (unless, of course, the point of your patch is to do clean up). You seem to have done this in other places as well. > >> DEF_HELPER_2(fctiduz, i64, env, i64) >> -#endif >> DEF_HELPER_2(frsp, i64, env, i64) >> DEF_HELPER_2(frin, i64, env, i64) >> DEF_HELPER_2(friz, i64, env, i64) >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >> index c07bb01..1fe82ce 100644 >> --- a/target-ppc/translate.c >> +++ b/target-ppc/translate.c >> @@ -2246,9 +2246,8 @@ GEN_FLOAT_B(ctiwz, 0x0F, 0x00, 0, PPC_FLOAT); >> GEN_FLOAT_B(ctiwuz, 0x0F, 0x04, 0, PPC2_FP_CVT_ISA206); >> /* frsp */ >> GEN_FLOAT_B(rsp, 0x0C, 0x00, 1, PPC_FLOAT); >> -#if defined(TARGET_PPC64) >> /* fcfid */ >> -GEN_FLOAT_B(cfid, 0x0E, 0x1A, 1, PPC_64B); >> +GEN_FLOAT_B(cfid, 0x0E, 0x1A, 1, PPC2_FP_CVT_S64); >> /* fcfids */ >> GEN_FLOAT_B(cfids, 0x0E, 0x1A, 0, PPC2_FP_CVT_ISA206); >> /* fcfidu */ >> @@ -2256,14 +2255,13 @@ GEN_FLOAT_B(cfidu, 0x0E, 0x1E, 0, PPC2_FP_CVT_ISA206); >> /* fcfidus */ >> GEN_FLOAT_B(cfidus, 0x0E, 0x1E, 0, PPC2_FP_CVT_ISA206); >> /* fctid */ >> -GEN_FLOAT_B(ctid, 0x0E, 0x19, 0, PPC_64B); >> +GEN_FLOAT_B(ctid, 0x0E, 0x19, 0, PPC2_FP_CVT_S64); >> +/* fctidz */ >> +GEN_FLOAT_B(ctidz, 0x0F, 0x19, 0, PPC2_FP_CVT_S64); >> /* fctidu */ >> GEN_FLOAT_B(ctidu, 0x0E, 0x1D, 0, PPC2_FP_CVT_ISA206); >> -/* fctidz */ >> -GEN_FLOAT_B(ctidz, 0x0F, 0x19, 0, PPC_64B); >> /* fctidu */ >> GEN_FLOAT_B(ctiduz, 0x0F, 0x1D, 0, PPC2_FP_CVT_ISA206); >> -#endif >> >> /* frin */ >> GEN_FLOAT_B(rin, 0x08, 0x0C, 1, PPC_FLOAT_EXT); >> @@ -10050,16 +10048,14 @@ GEN_HANDLER_E(fctiwu, 0x3F, 0x0E, 0x04, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> GEN_FLOAT_B(ctiwz, 0x0F, 0x00, 0, PPC_FLOAT), >> GEN_HANDLER_E(fctiwuz, 0x3F, 0x0F, 0x04, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> GEN_FLOAT_B(rsp, 0x0C, 0x00, 1, PPC_FLOAT), >> -#if defined(TARGET_PPC64) >> -GEN_FLOAT_B(cfid, 0x0E, 0x1A, 1, PPC_64B), >> +GEN_HANDLER_E(fcfid, 0x3F, 0x0E, 0x1A, 0x001F0000, PPC_NONE, PPC2_FP_CVT_S64), >> GEN_HANDLER_E(fcfids, 0x3B, 0x0E, 0x1A, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> GEN_HANDLER_E(fcfidu, 0x3F, 0x0E, 0x1E, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> GEN_HANDLER_E(fcfidus, 0x3B, 0x0E, 0x1E, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> -GEN_FLOAT_B(ctid, 0x0E, 0x19, 0, PPC_64B), >> +GEN_HANDLER_E(fctid, 0x3F, 0x0E, 0x19, 0x001F0000, PPC_NONE, PPC2_FP_CVT_S64), >> +GEN_HANDLER_E(fctidz, 0x3F, 0x0F, 0x19, 0x001F0000, PPC_NONE, PPC2_FP_CVT_S64), >> GEN_HANDLER_E(fctidu, 0x3F, 0x0E, 0x1D, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> -GEN_FLOAT_B(ctidz, 0x0F, 0x19, 0, PPC_64B), >> GEN_HANDLER_E(fctiduz, 0x3F, 0x0F, 0x1D, 0, PPC_NONE, PPC2_FP_CVT_ISA206), >> -#endif >> GEN_FLOAT_B(rin, 0x08, 0x0C, 1, PPC_FLOAT_EXT), >> GEN_FLOAT_B(riz, 0x08, 0x0D, 1, PPC_FLOAT_EXT), >> GEN_FLOAT_B(rip, 0x08, 0x0E, 1, PPC_FLOAT_EXT), >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index 48177ed..ac4d12a 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -5010,7 +5010,8 @@ POWERPC_FAMILY(e5500)(ObjectClass *oc, void *data) >> PPC_FLOAT_STFIWX | PPC_WAIT | >> PPC_MEM_TLBSYNC | PPC_TLBIVAX | PPC_MEM_SYNC | >> PPC_64B | PPC_POPCNTB | PPC_POPCNTWD; >> - pcc->insns_flags2 = PPC2_BOOKE206 | PPC2_PRCNTL | PPC2_PERM_ISA206; >> + pcc->insns_flags2 = PPC2_BOOKE206 | PPC2_PRCNTL | PPC2_PERM_ISA206 | \ >> + PPC2_FP_CVT_S64; >> pcc->msr_mask = (1ull << MSR_CM) | >> (1ull << MSR_GS) | >> (1ull << MSR_UCLE) | >> @@ -7906,6 +7907,7 @@ POWERPC_FAMILY(970)(ObjectClass *oc, void *data) >> PPC_MEM_TLBIE | PPC_MEM_TLBSYNC | >> PPC_64B | PPC_ALTIVEC | >> PPC_SEGMENT_64B | PPC_SLBI; >> + pcc->insns_flags2 = PPC2_FP_CVT_S64; >> pcc->msr_mask = (1ull << MSR_SF) | >> (1ull << MSR_VR) | >> (1ull << MSR_POW) | >> @@ -7958,6 +7960,7 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data) >> PPC_MEM_TLBIE | PPC_MEM_TLBSYNC | >> PPC_64B | >> PPC_SEGMENT_64B | PPC_SLBI; >> + pcc->insns_flags2 = PPC2_FP_CVT_S64; >> pcc->msr_mask = (1ull << MSR_SF) | >> (1ull << MSR_VR) | >> (1ull << MSR_POW) | >> @@ -8100,7 +8103,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data) >> pcc->insns_flags2 = PPC2_VSX | PPC2_DFP | PPC2_DBRX | PPC2_ISA205 | >> PPC2_PERM_ISA206 | PPC2_DIVE_ISA206 | >> PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206 | >> - PPC2_FP_TST_ISA206; >> + PPC2_FP_TST_ISA206 | PPC2_FP_CVT_S64; >> pcc->msr_mask = (1ull << MSR_SF) | >> (1ull << MSR_VR) | >> (1ull << MSR_VSX) | >> @@ -8178,7 +8181,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) >> PPC2_ATOMIC_ISA206 | PPC2_FP_CVT_ISA206 | >> PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207 | >> PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 | >> - PPC2_ISA205 | PPC2_ISA207S; >> + PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64; >> pcc->msr_mask = (1ull << MSR_SF) | >> (1ull << MSR_TM) | >> (1ull << MSR_VR) | >> > > Other than the minor comments .... > > Reviewed-by: Tom Musta > Tested-by: Tom Musta > One more issue .... the patch is missing the signoff (Alex will need one, I believe)