[Qemu-devel] [PATCH v3] target/ppc: Fix carry flag setting for shift algebraic instructions

[Qemu-devel] [PATCH v3] target/ppc: Fix carry flag setting for shift algebraic instructions

[Sandipan Das]

For POWER ISA v3.0, the XER bit CA32 needs to be set by the shift
right algebraic instructions whenever the CA bit is to be set. This
change affects the following instructions:
  * Shift Right Algebraic Word (sraw[.])
  * Shift Right Algebraic Word Immediate (srawi[.])
  * Shift Right Algebraic Doubleword (srad[.])
  * Shift Right Algebraic Doubleword Immediate (sradi[.])

Signed-off-by: Sandipan Das <sandipan@linux.vnet.ibm.com>
---
v2: Add tcg_temp_free() required in gen_sraw() and gen_srad()

v3: Remove explicit checking for ISA v3.0 when setting CA32
---
 target/ppc/int_helper.c | 8 ++++++++
 target/ppc/translate.c  | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index da4e1a62c9..0bdd96aebe 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -231,6 +231,10 @@ target_ulong helper_sraw(CPUPPCState *env, target_ulong value,
         ret = (int32_t)value >> 31;
         env->ca = (ret != 0);
     }
+
+    /* update CA32 for ISA v3.0 */
+    env->ca32 = env->ca;
+
     return (target_long)ret;
 }
 
@@ -257,6 +261,10 @@ target_ulong helper_srad(CPUPPCState *env, target_ulong value,
         ret = (int64_t)value >> 63;
         env->ca = (ret != 0);
     }
+
+    /* update CA32 for ISA v3.0 */
+    env->ca32 = env->ca;
+
     return ret;
 }
 #endif
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 606b605ba0..c35a2027eb 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -2192,6 +2192,10 @@ static void gen_srawi(DisasContext *ctx)
         tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
         tcg_gen_sari_tl(dst, dst, sh);
     }
+
+    /* update CA32 for ISA v3.0 */
+    tcg_gen_mov_tl(cpu_ca32, cpu_ca);
+
     if (unlikely(Rc(ctx->opcode) != 0)) {
         gen_set_Rc0(ctx, dst);
     }
@@ -2269,6 +2273,10 @@ static inline void gen_sradi(DisasContext *ctx, int n)
         tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
         tcg_gen_sari_tl(dst, src, sh);
     }
+
+    /* update CA32 for ISA v3.0 */
+    tcg_gen_mov_tl(cpu_ca32, cpu_ca);
+
     if (unlikely(Rc(ctx->opcode) != 0)) {
         gen_set_Rc0(ctx, dst);
     }
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3] target/ppc: Fix carry flag setting for shift algebraic instructions

[Richard Henderson]

On 10/03/2017 02:23 AM, Sandipan Das wrote:
> @@ -231,6 +231,10 @@ target_ulong helper_sraw(CPUPPCState *env, target_ulong value,
>          ret = (int32_t)value >> 31;
>          env->ca = (ret != 0);
>      }
> +
> +    /* update CA32 for ISA v3.0 */
> +    env->ca32 = env->ca;

As I said before, modify ca32 only when ca is modified.
E.g.

  env->ca32 = env->ca = (ret != 0);

> @@ -257,6 +261,10 @@ target_ulong helper_srad(CPUPPCState *env, target_ulong value,
>          ret = (int64_t)value >> 63;
>          env->ca = (ret != 0);
>      }
> +
> +    /* update CA32 for ISA v3.0 */
> +    env->ca32 = env->ca;

Likewise.

> @@ -2192,6 +2192,10 @@ static void gen_srawi(DisasContext *ctx)
>          tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
>          tcg_gen_sari_tl(dst, dst, sh);
>      }
> +
> +    /* update CA32 for ISA v3.0 */
> +    tcg_gen_mov_tl(cpu_ca32, cpu_ca);

Likewise.

> @@ -2269,6 +2273,10 @@ static inline void gen_sradi(DisasContext *ctx, int n)
>          tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
>          tcg_gen_sari_tl(dst, src, sh);
>      }
> +
> +    /* update CA32 for ISA v3.0 */
> +    tcg_gen_mov_tl(cpu_ca32, cpu_ca);

Likewise.


r~

Re: [Qemu-devel] [PATCH v3] target/ppc: Fix carry flag setting for shift algebraic instructions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On Thu, Oct 05, 2017 at 08:42:56AM -0400, Richard Henderson wrote:
> On 10/03/2017 02:23 AM, Sandipan Das wrote:
> > @@ -231,6 +231,10 @@ target_ulong helper_sraw(CPUPPCState *env, target_ulong value,
> >          ret = (int32_t)value >> 31;
> >          env->ca = (ret != 0);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    env->ca32 = env->ca;
> 
> As I said before, modify ca32 only when ca is modified.
> E.g.
> 
>   env->ca32 = env->ca = (ret != 0);
> 
> > @@ -257,6 +261,10 @@ target_ulong helper_srad(CPUPPCState *env, target_ulong value,
> >          ret = (int64_t)value >> 63;
> >          env->ca = (ret != 0);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    env->ca32 = env->ca;
> 
> Likewise.
> 
> > @@ -2192,6 +2192,10 @@ static void gen_srawi(DisasContext *ctx)
> >          tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
> >          tcg_gen_sari_tl(dst, dst, sh);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    tcg_gen_mov_tl(cpu_ca32, cpu_ca);
> 
> Likewise.

Also, for the helper functions it definitely makes sense to always set
CA32 when CA is set, regardless of CPU model, it's close enough to
free.  When we're generating code, however, the trade-off is
different, we only need to test the CPU model at translate time, but
we need to execute the generated instrucitons potentially more times.

So I'm wondering if for the gen_* functions we _should_ be checking
for ISA300 before generating the CA32 update instructions.

> 
> > @@ -2269,6 +2273,10 @@ static inline void gen_sradi(DisasContext *ctx, int n)
> >          tcg_gen_setcondi_tl(TCG_COND_NE, cpu_ca, cpu_ca, 0);
> >          tcg_gen_sari_tl(dst, src, sh);
> >      }
> > +
> > +    /* update CA32 for ISA v3.0 */
> > +    tcg_gen_mov_tl(cpu_ca32, cpu_ca);
> 
> Likewise.
> 
> 
> r~
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]