diff mbox series

[v5,23/23] target/ppc: Move cmp/cmpi/cmpl/cmpli to decodetree

Message ID 20210517205025.3777947-24-matheus.ferst@eldorado.org.br (mailing list archive)
State New, archived
Headers show
Series Base for adding PowerPC 64-bit instructions | expand

Commit Message

Matheus K. Ferst May 17, 2021, 8:50 p.m. UTC
From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/insn32.decode                   | 14 ++++++
 target/ppc/translate.c                     | 52 ----------------------
 target/ppc/translate/fixedpoint-impl.c.inc | 31 +++++++++++++
 3 files changed, 45 insertions(+), 52 deletions(-)

Comments

David Gibson May 18, 2021, 12:56 a.m. UTC | #1
On Mon, May 17, 2021 at 05:50:25PM -0300, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/insn32.decode                   | 14 ++++++
>  target/ppc/translate.c                     | 52 ----------------------
>  target/ppc/translate/fixedpoint-impl.c.inc | 31 +++++++++++++
>  3 files changed, 45 insertions(+), 52 deletions(-)
> 
> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
> index 93e5d44d9e..9fd8d6b817 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -20,6 +20,10 @@
>  &D              rt ra si:int64_t
>  @D              ...... rt:5 ra:5 si:s16                 &D
>  
> +&D_bf           bf l:bool ra imm
> +@D_bfs          ...... bf:3 - l:1 ra:5 imm:s16          &D_bf
> +@D_bfu          ...... bf:3 - l:1 ra:5 imm:16           &D_bf
> +
>  %ds_si          2:s14  !function=times_4
>  @DS             ...... rt:5 ra:5 .............. ..      &D si=%ds_si
>  
> @@ -36,6 +40,9 @@
>  &X_bi           rt bi
>  @X_bi           ...... rt:5 bi:5 ----- .......... -     &X_bi
>  
> +&X_bfl          bf l:bool ra rb
> +@X_bfl          ...... bf:3 - l:1 ra:5 rb:5 ..........- &X_bfl
> +
>  ### Fixed-Point Load Instructions
>  
>  LBZ             100010 ..... ..... ................     @D
> @@ -89,6 +96,13 @@ STDU            111110 ..... ..... ..............01     @DS
>  STDX            011111 ..... ..... ..... 0010010101 -   @X
>  STDUX           011111 ..... ..... ..... 0010110101 -   @X
>  
> +### Fixed-Point Compare Instructions
> +
> +CMP             011111 ... - . ..... ..... 0000000000 - @X_bfl
> +CMPL            011111 ... - . ..... ..... 0000100000 - @X_bfl
> +CMPI            001011 ... - . ..... ................   @D_bfs
> +CMPLI           001010 ... - . ..... ................   @D_bfu
> +
>  ### Fixed-Point Arithmetic Instructions
>  
>  ADDI            001110 ..... ..... ................     @D
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index aef01af396..3fe58d0386 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -1575,54 +1575,6 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
>      }
>  }
>  
> -/* cmp */
> -static void gen_cmp(DisasContext *ctx)
> -{
> -    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
> -        gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
> -                   1, crfD(ctx->opcode));
> -    } else {
> -        gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
> -                     1, crfD(ctx->opcode));
> -    }
> -}
> -
> -/* cmpi */
> -static void gen_cmpi(DisasContext *ctx)
> -{
> -    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
> -        gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
> -                    1, crfD(ctx->opcode));
> -    } else {
> -        gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
> -                      1, crfD(ctx->opcode));
> -    }
> -}
> -
> -/* cmpl */
> -static void gen_cmpl(DisasContext *ctx)
> -{
> -    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
> -        gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
> -                   0, crfD(ctx->opcode));
> -    } else {
> -        gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
> -                     0, crfD(ctx->opcode));
> -    }
> -}
> -
> -/* cmpli */
> -static void gen_cmpli(DisasContext *ctx)
> -{
> -    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
> -        gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
> -                    0, crfD(ctx->opcode));
> -    } else {
> -        gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
> -                      0, crfD(ctx->opcode));
> -    }
> -}
> -
>  /* cmprb - range comparison: isupper, isaplha, islower*/
>  static void gen_cmprb(DisasContext *ctx)
>  {
> @@ -7725,10 +7677,6 @@ GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0x0000F801, PPC_NONE, PPC2_ISA310),
>  GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0x0000F801, PPC_NONE, PPC2_ISA310),
>  #endif
>  GEN_HANDLER(invalid, 0x00, 0x00, 0x00, 0xFFFFFFFF, PPC_NONE),
> -GEN_HANDLER(cmp, 0x1F, 0x00, 0x00, 0x00400000, PPC_INTEGER),
> -GEN_HANDLER(cmpi, 0x0B, 0xFF, 0xFF, 0x00400000, PPC_INTEGER),
> -GEN_HANDLER(cmpl, 0x1F, 0x00, 0x01, 0x00400001, PPC_INTEGER),
> -GEN_HANDLER(cmpli, 0x0A, 0xFF, 0xFF, 0x00400000, PPC_INTEGER),
>  #if defined(TARGET_PPC64)
>  GEN_HANDLER_E(cmpeqb, 0x1F, 0x00, 0x07, 0x00600000, PPC_NONE, PPC2_ISA300),
>  #endif
> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> index 4f257a931c..49c8993333 100644
> --- a/target/ppc/translate/fixedpoint-impl.c.inc
> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> @@ -165,6 +165,37 @@ TRANS64(STDU, do_ldst_D, true, true, MO_Q)
>  TRANS64(STDUX, do_ldst_X, true, true, MO_Q)
>  TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
>  
> +/*
> + * Fixed-Point Compare Instructions
> + */
> +
> +static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
> +{
> +    REQUIRE_INSNS_FLAGS(ctx, INTEGER);
> +    if(a->l && (ctx->insns_flags & PPC_64B)) {
> +        gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> +    } else {
> +        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> +    }
> +    return true;
> +}
> +
> +static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
> +{
> +    REQUIRE_INSNS_FLAGS(ctx, INTEGER);
> +    if(a->l && (ctx->insns_flags & PPC_64B)) {
> +        gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> +    } else {
> +        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> +    }
> +    return true;
> +}
> +
> +TRANS(CMP, do_cmp_X, true);
> +TRANS(CMPL, do_cmp_X, false);
> +TRANS(CMPI, do_cmp_D, true);
> +TRANS(CMPLI, do_cmp_D, false);
> +
>  /*
>   * Fixed-Point Arithmetic Instructions
>   */
Richard Henderson May 18, 2021, 10:12 a.m. UTC | #2
On 5/17/21 3:50 PM, matheus.ferst@eldorado.org.br wrote:
> +    if(a->l && (ctx->insns_flags & PPC_64B)) {

Space after IF.

If I look back to the 6xx manual, I see

   NOTE: If L = 1, the instruction form is invalid.

The fact that we're allowing L=1 for ppc32 is an existing bug, afaics.  We 
should fix that.


r~
Matheus K. Ferst May 21, 2021, 5:25 p.m. UTC | #3
On 18/05/2021 07:12, Richard Henderson wrote:
> On 5/17/21 3:50 PM, matheus.ferst@eldorado.org.br wrote:
>> +    if(a->l && (ctx->insns_flags & PPC_64B)) {
> 
> Space after IF.
> > If I look back to the 6xx manual, I see
> 
>    NOTE: If L = 1, the instruction form is invalid.
> 
> The fact that we're allowing L=1 for ppc32 is an existing bug, afaics.  
> We should fix that.
> 
> 
> r~

The previous commit on this line in translate.c says that "on most 32bit 
CPUs we should always treat the compare as 32bit compare, as the CPU 
will ignore the L bit", so maybe it was intentional. Should we change it 
anyway?
Richard Henderson May 24, 2021, 6:51 p.m. UTC | #4
On 5/21/21 10:25 AM, Matheus K. Ferst wrote:
> On 18/05/2021 07:12, Richard Henderson wrote:
>> On 5/17/21 3:50 PM, matheus.ferst@eldorado.org.br wrote:
>>> +    if(a->l && (ctx->insns_flags & PPC_64B)) {
>>
>> Space after IF.
>> > If I look back to the 6xx manual, I see
>>
>>    NOTE: If L = 1, the instruction form is invalid.
>>
>> The fact that we're allowing L=1 for ppc32 is an existing bug, afaics. We 
>> should fix that.
>>
>>
>> r~
> 
> The previous commit on this line in translate.c says that "on most 32bit CPUs 
> we should always treat the compare as 32bit compare, as the CPU will ignore the 
> L bit", so maybe it was intentional. Should we change it anyway?

The actual change of 36f48d9c78c is about NARROW_MODE, which is about the 
MSR.SF bit, and is correct.

The commit message mentions the e500mc specifically does check the L bit, and 
then hand-waves about the others not checking.  But the text I found in the 6xx 
manual says that one checks too.

I wonder if the IBM folk can shed any further light on this?


r~
Matheus K. Ferst May 26, 2021, 3:17 p.m. UTC | #5
On 24/05/2021 15:51, Richard Henderson wrote:
> On 5/21/21 10:25 AM, Matheus K. Ferst wrote:
>> On 18/05/2021 07:12, Richard Henderson wrote:
>>> On 5/17/21 3:50 PM, matheus.ferst@eldorado.org.br wrote:
>>>> +    if(a->l && (ctx->insns_flags & PPC_64B)) {
>>>
>>> Space after IF.
>>> > If I look back to the 6xx manual, I see
>>>
>>>    NOTE: If L = 1, the instruction form is invalid.
>>>
>>> The fact that we're allowing L=1 for ppc32 is an existing bug, 
>>> afaics. We should fix that.
>>>
>>>
>>> r~
>>
>> The previous commit on this line in translate.c says that "on most 
>> 32bit CPUs we should always treat the compare as 32bit compare, as the 
>> CPU will ignore the L bit", so maybe it was intentional. Should we 
>> change it anyway?
> 
> The actual change of 36f48d9c78c is about NARROW_MODE, which is about 
> the MSR.SF bit, and is correct.
> 
> The commit message mentions the e500mc specifically does check the L 
> bit, and then hand-waves about the others not checking.  But the text I 
> found in the 6xx manual says that one checks too.
> 
> I wonder if the IBM folk can shed any further light on this?
> 
> 
> r~

I was pointed to the 601 manual, which says:

"While the PowerPC architecture specifies that the value in the L field 
determines whether the operands are treated as 32- or 64-bit values, the 
601 ignores the value in the L field and treats the operands as 32-bit 
values."

There is also a section in Appendix B called "Reserved Bits in 
Instructions", which says:

"These are shown with '/'s in the instruction opcode definitions. In the 
POWER architecture such bits are ignored by the processor. In PowerPC 
architecture they must be 0 or the instruction form is invalid. In 
several cases the PowerPC architecture assumes that such bits in POWER 
instructions are indeed 0. The cases include the following:
- cmpi, cmp, cmpli, and cmpl assume that bit 10 in the POWER 
instructions is 0.
- mtspr and mfspr assume that bits 16–20 in the POWER instructions are 0."

Searching the manuals for other processors, I identified that the 
manuals for 405, 440, e500, and e500mc explicit says that the L bit 
should always be 0, and manuals for 603e, 604, 604e, 740/745/750/755, 
750CX, 750CL, 750FX, 7400/7410, 7447/7447A/7448/7450/7455, e300, and 
e600 list the bit L in operand syntax but do not mention any 
restrictions on its value.

Alfredo Dal Ava Junior (adalva) did some tests for us on his G4 MacBook, 
confirming that the bit is ignored in PowerPC 7447A v1.2, one of which 
the manual does not specify the behavior, but I don't know if can assume 
the same for other processors.

If we do bother to emulate the specific behavior for each CPU, what 
would be the default for those whose manual is not explicit and we 
cannot test? Also, I not sure how to check for it, do we need a new 
POWERPC_FLAG in pcc->flags?
Richard Henderson May 26, 2021, 4:11 p.m. UTC | #6
On 5/26/21 8:17 AM, Matheus K. Ferst wrote:
> On 24/05/2021 15:51, Richard Henderson wrote:
>> On 5/21/21 10:25 AM, Matheus K. Ferst wrote:
>>> On 18/05/2021 07:12, Richard Henderson wrote:
>>>> On 5/17/21 3:50 PM, matheus.ferst@eldorado.org.br wrote:
>>>>> +    if(a->l && (ctx->insns_flags & PPC_64B)) {
>>>>
>>>> Space after IF.
>>>> > If I look back to the 6xx manual, I see
>>>>
>>>>    NOTE: If L = 1, the instruction form is invalid.
>>>>
>>>> The fact that we're allowing L=1 for ppc32 is an existing bug, afaics. We 
>>>> should fix that.
>>>>
>>>>
>>>> r~
>>>
>>> The previous commit on this line in translate.c says that "on most 32bit 
>>> CPUs we should always treat the compare as 32bit compare, as the CPU will 
>>> ignore the L bit", so maybe it was intentional. Should we change it anyway?
>>
>> The actual change of 36f48d9c78c is about NARROW_MODE, which is about the 
>> MSR.SF bit, and is correct.
>>
>> The commit message mentions the e500mc specifically does check the L bit, and 
>> then hand-waves about the others not checking.  But the text I found in the 
>> 6xx manual says that one checks too.
>>
>> I wonder if the IBM folk can shed any further light on this?
>>
>>
>> r~
> 
> I was pointed to the 601 manual, which says:
> 
> "While the PowerPC architecture specifies that the value in the L field 
> determines whether the operands are treated as 32- or 64-bit values, the 601 
> ignores the value in the L field and treats the operands as 32-bit values."
> 
> There is also a section in Appendix B called "Reserved Bits in Instructions", 
> which says:
> 
> "These are shown with '/'s in the instruction opcode definitions. In the POWER 
> architecture such bits are ignored by the processor. In PowerPC architecture 
> they must be 0 or the instruction form is invalid. In several cases the PowerPC 
> architecture assumes that such bits in POWER instructions are indeed 0. The 
> cases include the following:
> - cmpi, cmp, cmpli, and cmpl assume that bit 10 in the POWER instructions is 0.
> - mtspr and mfspr assume that bits 16–20 in the POWER instructions are 0."
> 
> Searching the manuals for other processors, I identified that the manuals for 
> 405, 440, e500, and e500mc explicit says that the L bit should always be 0, and 
> manuals for 603e, 604, 604e, 740/745/750/755, 750CX, 750CL, 750FX, 7400/7410, 
> 7447/7447A/7448/7450/7455, e300, and e600 list the bit L in operand syntax but 
> do not mention any restrictions on its value.
> 
> Alfredo Dal Ava Junior (adalva) did some tests for us on his G4 MacBook, 
> confirming that the bit is ignored in PowerPC 7447A v1.2, one of which the 
> manual does not specify the behavior, but I don't know if can assume the same 
> for other processors.
> 
> If we do bother to emulate the specific behavior for each CPU, what would be 
> the default for those whose manual is not explicit and we cannot test? Also, I 
> not sure how to check for it, do we need a new POWERPC_FLAG in pcc->flags?

Thanks for the research.

There's an argument for following the architecture, even when implementations 
vary.  Especially when implementations very, as this makes testing with qemu 
more likely to catch software bugs.

There's another argument for following implementations.  I would generally 
reserve this interpretation for historical cpus, where we are trying to emulate 
something specific (e.g. a games console) where the legacy software relies on 
specific behavior.

I'll let David have the final call on this, but my inclination is to follow the 
architecture and require 0s for reserved bits.


r~
David Gibson May 27, 2021, 1:11 a.m. UTC | #7
On Wed, May 26, 2021 at 12:17:48PM -0300, Matheus K. Ferst wrote:
> On 24/05/2021 15:51, Richard Henderson wrote:
> > On 5/21/21 10:25 AM, Matheus K. Ferst wrote:
> > > On 18/05/2021 07:12, Richard Henderson wrote:
> > > > On 5/17/21 3:50 PM, matheus.ferst@eldorado.org.br wrote:
> > > > > +    if(a->l && (ctx->insns_flags & PPC_64B)) {
> > > > 
> > > > Space after IF.
> > > > > If I look back to the 6xx manual, I see
> > > > 
> > > >    NOTE: If L = 1, the instruction form is invalid.
> > > > 
> > > > The fact that we're allowing L=1 for ppc32 is an existing bug,
> > > > afaics. We should fix that.
> > > > 
> > > > 
> > > > r~
> > > 
> > > The previous commit on this line in translate.c says that "on most
> > > 32bit CPUs we should always treat the compare as 32bit compare, as
> > > the CPU will ignore the L bit", so maybe it was intentional. Should
> > > we change it anyway?
> > 
> > The actual change of 36f48d9c78c is about NARROW_MODE, which is about
> > the MSR.SF bit, and is correct.
> > 
> > The commit message mentions the e500mc specifically does check the L
> > bit, and then hand-waves about the others not checking.  But the text I
> > found in the 6xx manual says that one checks too.
> > 
> > I wonder if the IBM folk can shed any further light on this?
> > 
> > 
> > r~
> 
> I was pointed to the 601 manual, which says:
> 
> "While the PowerPC architecture specifies that the value in the L field
> determines whether the operands are treated as 32- or 64-bit values, the 601
> ignores the value in the L field and treats the operands as 32-bit values."
> 
> There is also a section in Appendix B called "Reserved Bits in
> Instructions", which says:
> 
> "These are shown with '/'s in the instruction opcode definitions. In the
> POWER architecture such bits are ignored by the processor. In PowerPC
> architecture they must be 0 or the instruction form is invalid. In several
> cases the PowerPC architecture assumes that such bits in POWER instructions
> are indeed 0. The cases include the following:
> - cmpi, cmp, cmpli, and cmpl assume that bit 10 in the POWER instructions is
> 0.
> - mtspr and mfspr assume that bits 16–20 in the POWER instructions are 0."
> 
> Searching the manuals for other processors, I identified that the manuals
> for 405, 440, e500, and e500mc explicit says that the L bit should always be
> 0, and manuals for 603e, 604, 604e, 740/745/750/755, 750CX, 750CL, 750FX,
> 7400/7410, 7447/7447A/7448/7450/7455, e300, and e600 list the bit L in
> operand syntax but do not mention any restrictions on its value.
> 
> Alfredo Dal Ava Junior (adalva) did some tests for us on his G4 MacBook,
> confirming that the bit is ignored in PowerPC 7447A v1.2, one of which the
> manual does not specify the behavior, but I don't know if can assume the
> same for other processors.
> 
> If we do bother to emulate the specific behavior for each CPU, what would be
> the default for those whose manual is not explicit and we cannot test? Also,
> I not sure how to check for it, do we need a new POWERPC_FLAG in pcc->flags?

My inclination would be to make L=1 program check on all 32-bit cpus
for now, and if someone pipes up with a guest broken because it
assumes L=1 is ignored, we can fix it then.
diff mbox series

Patch

diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index 93e5d44d9e..9fd8d6b817 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -20,6 +20,10 @@ 
 &D              rt ra si:int64_t
 @D              ...... rt:5 ra:5 si:s16                 &D
 
+&D_bf           bf l:bool ra imm
+@D_bfs          ...... bf:3 - l:1 ra:5 imm:s16          &D_bf
+@D_bfu          ...... bf:3 - l:1 ra:5 imm:16           &D_bf
+
 %ds_si          2:s14  !function=times_4
 @DS             ...... rt:5 ra:5 .............. ..      &D si=%ds_si
 
@@ -36,6 +40,9 @@ 
 &X_bi           rt bi
 @X_bi           ...... rt:5 bi:5 ----- .......... -     &X_bi
 
+&X_bfl          bf l:bool ra rb
+@X_bfl          ...... bf:3 - l:1 ra:5 rb:5 ..........- &X_bfl
+
 ### Fixed-Point Load Instructions
 
 LBZ             100010 ..... ..... ................     @D
@@ -89,6 +96,13 @@  STDU            111110 ..... ..... ..............01     @DS
 STDX            011111 ..... ..... ..... 0010010101 -   @X
 STDUX           011111 ..... ..... ..... 0010110101 -   @X
 
+### Fixed-Point Compare Instructions
+
+CMP             011111 ... - . ..... ..... 0000000000 - @X_bfl
+CMPL            011111 ... - . ..... ..... 0000100000 - @X_bfl
+CMPI            001011 ... - . ..... ................   @D_bfs
+CMPLI           001010 ... - . ..... ................   @D_bfu
+
 ### Fixed-Point Arithmetic Instructions
 
 ADDI            001110 ..... ..... ................     @D
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index aef01af396..3fe58d0386 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1575,54 +1575,6 @@  static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg)
     }
 }
 
-/* cmp */
-static void gen_cmp(DisasContext *ctx)
-{
-    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
-        gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
-                   1, crfD(ctx->opcode));
-    } else {
-        gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
-                     1, crfD(ctx->opcode));
-    }
-}
-
-/* cmpi */
-static void gen_cmpi(DisasContext *ctx)
-{
-    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
-        gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
-                    1, crfD(ctx->opcode));
-    } else {
-        gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode),
-                      1, crfD(ctx->opcode));
-    }
-}
-
-/* cmpl */
-static void gen_cmpl(DisasContext *ctx)
-{
-    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
-        gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
-                   0, crfD(ctx->opcode));
-    } else {
-        gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],
-                     0, crfD(ctx->opcode));
-    }
-}
-
-/* cmpli */
-static void gen_cmpli(DisasContext *ctx)
-{
-    if ((ctx->opcode & 0x00200000) && (ctx->insns_flags & PPC_64B)) {
-        gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
-                    0, crfD(ctx->opcode));
-    } else {
-        gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode),
-                      0, crfD(ctx->opcode));
-    }
-}
-
 /* cmprb - range comparison: isupper, isaplha, islower*/
 static void gen_cmprb(DisasContext *ctx)
 {
@@ -7725,10 +7677,6 @@  GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0x0000F801, PPC_NONE, PPC2_ISA310),
 GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0x0000F801, PPC_NONE, PPC2_ISA310),
 #endif
 GEN_HANDLER(invalid, 0x00, 0x00, 0x00, 0xFFFFFFFF, PPC_NONE),
-GEN_HANDLER(cmp, 0x1F, 0x00, 0x00, 0x00400000, PPC_INTEGER),
-GEN_HANDLER(cmpi, 0x0B, 0xFF, 0xFF, 0x00400000, PPC_INTEGER),
-GEN_HANDLER(cmpl, 0x1F, 0x00, 0x01, 0x00400001, PPC_INTEGER),
-GEN_HANDLER(cmpli, 0x0A, 0xFF, 0xFF, 0x00400000, PPC_INTEGER),
 #if defined(TARGET_PPC64)
 GEN_HANDLER_E(cmpeqb, 0x1F, 0x00, 0x07, 0x00600000, PPC_NONE, PPC2_ISA300),
 #endif
diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
index 4f257a931c..49c8993333 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -165,6 +165,37 @@  TRANS64(STDU, do_ldst_D, true, true, MO_Q)
 TRANS64(STDUX, do_ldst_X, true, true, MO_Q)
 TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
 
+/*
+ * Fixed-Point Compare Instructions
+ */
+
+static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
+{
+    REQUIRE_INSNS_FLAGS(ctx, INTEGER);
+    if(a->l && (ctx->insns_flags & PPC_64B)) {
+        gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
+    } else {
+        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
+    }
+    return true;
+}
+
+static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
+{
+    REQUIRE_INSNS_FLAGS(ctx, INTEGER);
+    if(a->l && (ctx->insns_flags & PPC_64B)) {
+        gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
+    } else {
+        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
+    }
+    return true;
+}
+
+TRANS(CMP, do_cmp_X, true);
+TRANS(CMPL, do_cmp_X, false);
+TRANS(CMPI, do_cmp_D, true);
+TRANS(CMPLI, do_cmp_D, false);
+
 /*
  * Fixed-Point Arithmetic Instructions
  */