On 9/16/19 1:02 PM, Paul A. Clarke wrote: > +#define FP_DRN2 (1ull << FPSCR_DRN2) > +#define FP_DRN1 (1ull << FPSCR_DRN1) > +#define FP_DRN0 (1ull << FPSCR_DRN0) > +#define FP_DRN (FP_DRN2 | FP_DRN1 | FP_DRN0)
Why not just 7ull << FPSCR_DRN? Are the individual DRN bits separately useful? They don't appear to be... > -#define FP_MODE FP_RN > +#define FP_MODE (FP_DRN | FP_RN) This, I think, isn't helpful. > +static void gen_helper_mffscrn(DisasContext *ctx, TCGv_i64 t1) > +{ > + TCGv_i64 t0 = tcg_temp_new_i64(); > + TCGv_i32 mask = tcg_const_i32(0x0001); > + > + gen_reset_fpstatus(); > + tcg_gen_extu_tl_i64(t0, cpu_fpscr); > + tcg_gen_andi_i64(t0, t0, FP_MODE | FP_ENABLES); > + set_fpr(rD(ctx->opcode), t0); > + > + /* Mask FPSCR value to clear RN. */ > + tcg_gen_andi_i64(t0, t0, ~FP_MODE); Because here, > +static void gen_mffscrn(DisasContext *ctx) > +{ > + TCGv_i64 t1; > + > + if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) { > + return gen_mffs(ctx); > + } > + > + if (unlikely(!ctx->fpu_enabled)) { > + gen_exception(ctx, POWERPC_EXCP_FPU); > + return; > + } > + > + t1 = tcg_temp_new_i64(); > + get_fpr(t1, rB(ctx->opcode)); > + /* Mask FRB to get just RN. */ > + tcg_gen_andi_i64(t1, t1, FP_MODE); and here, we're only interested in RN, not DRN. Possibly FP_MODE should itself be removed. It's used exactly once, in mffsl, which could just as easily reference FP_RN | FP_DRN. r~