Patchwork Incorrect handling of more PPC64 insns (PATCH)

login
register
mail settings
Submitter Torbjorn Granlund
Date May 7, 2013, 3:58 p.m.
Message ID <86fvxypyru.fsf_-_@shell.gmplib.org>
Download mbox | patch
Permalink /patch/242387/
State New
Headers show

Comments

Torbjorn Granlund - May 7, 2013, 3:58 p.m.
OK, so took to reading some of translate to see how well it agrees with
the PPC architecture definition.

I spotted a bug with cmp, which was repeated 4 times.  Somebody decided
that NARROW_MODE should affect the handling of cmp instructions, which
is contrary to the ISA documentation.

The first hunk is just a comment about suspicious code.  I don't suggest
to apply that.

Incidentally, this patch makes GMP testing go a bit further, and the
testcase bug-qemu-ppc-again.s works correctly.
Alexander Graf - May 7, 2013, 5:12 p.m.
On 05/07/2013 05:58 PM, Torbjorn Granlund wrote:
> OK, so took to reading some of translate to see how well it agrees with
> the PPC architecture definition.
>
> I spotted a bug with cmp, which was repeated 4 times.  Somebody decided
> that NARROW_MODE should affect the handling of cmp instructions, which
> is contrary to the ISA documentation.
>
> The first hunk is just a comment about suspicious code.  I don't suggest
> to apply that.
>
> Incidentally, this patch makes GMP testing go a bit further, and the
> testcase bug-qemu-ppc-again.s works correctly.

Richard, could you please proof read this?


Thanks!

Alex

>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 1a84653..c44b96d 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -665,6 +665,7 @@ static inline void gen_op_cmpi32(TCGv arg0, target_ulong arg1, int s, int crf)
>
>   static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
>   {
> +// suspicious code -- tege
>       if (NARROW_MODE(ctx)) {
>           gen_op_cmpi32(reg, 0, 1, 0);
>       } else {
> @@ -675,7 +676,7 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
>   /* cmp */
>   static void gen_cmp(DisasContext *ctx)
>   {
> -    if (NARROW_MODE(ctx) || !(ctx->opcode&  0x00200000)) {
> +    if (!(ctx->opcode&  0x00200000)) {
>           gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
>                        1, crfD(ctx->opcode));
>       } else {
> @@ -687,7 +688,7 @@ static void gen_cmp(DisasContext *ctx)
>   /* cmpi */
>   static void gen_cmpi(DisasContext *ctx)
>   {
> -    if (NARROW_MODE(ctx) || !(ctx->opcode&  0x00200000)) {
> +    if (!(ctx->opcode&  0x00200000)) {
>           gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
>                         1, crfD(ctx->opcode));
>       } else {
> @@ -699,7 +700,7 @@ static void gen_cmpi(DisasContext *ctx)
>   /* cmpl */
>   static void gen_cmpl(DisasContext *ctx)
>   {
> -    if (NARROW_MODE(ctx) || !(ctx->opcode&  0x00200000)) {
> +    if (!(ctx->opcode&  0x00200000)) {
>           gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
>                        0, crfD(ctx->opcode));
>       } else {
> @@ -711,7 +712,7 @@ static void gen_cmpl(DisasContext *ctx)
>   /* cmpli */
>   static void gen_cmpli(DisasContext *ctx)
>   {
> -    if (NARROW_MODE(ctx) || !(ctx->opcode&  0x00200000)) {
> +    if (!(ctx->opcode&  0x00200000)) {
>           gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
>                         0, crfD(ctx->opcode));
>       } else {
>
Torbjorn Granlund - May 7, 2013, 6:10 p.m.
Alexander Graf <agraf@suse.de> writes:

  > The first hunk is just a comment about suspicious code.  I don't suggest
  > to apply that.

The "suspicious" code is correct.  The Rc update should indeed be
SF-mode dependent.

With the other 4 hunks, qemu-ppc64 is now able to execute GMP's
testsuite to completion, using a shared lib build.  (The static build
has yet to complete.)

Patch

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 1a84653..c44b96d 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -665,6 +665,7 @@  static inline void gen_op_cmpi32(TCGv arg0, target_ulong arg1, int s, int crf)
 
 static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
 {
+// suspicious code -- tege
     if (NARROW_MODE(ctx)) {
         gen_op_cmpi32(reg, 0, 1, 0);
     } else {
@@ -675,7 +676,7 @@  static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
 /* cmp */
 static void gen_cmp(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (!(ctx->opcode & 0x00200000)) {
         gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
                      1, crfD(ctx->opcode));
     } else {
@@ -687,7 +688,7 @@  static void gen_cmp(DisasContext *ctx)
 /* cmpi */
 static void gen_cmpi(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (!(ctx->opcode & 0x00200000)) {
         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
                       1, crfD(ctx->opcode));
     } else {
@@ -699,7 +700,7 @@  static void gen_cmpi(DisasContext *ctx)
 /* cmpl */
 static void gen_cmpl(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (!(ctx->opcode & 0x00200000)) {
         gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
                      0, crfD(ctx->opcode));
     } else {
@@ -711,7 +712,7 @@  static void gen_cmpl(DisasContext *ctx)
 /* cmpli */
 static void gen_cmpli(DisasContext *ctx)
 {
-    if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) {
+    if (!(ctx->opcode & 0x00200000)) {
         gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
                       0, crfD(ctx->opcode));
     } else {