diff mbox series

[v3,25/30] target/ppc: Move ADDI, ADDIS to decodetree, implement PADDI

Message ID 20210430011543.1017113-26-richard.henderson@linaro.org
State New
Headers show
Series Base for adding PowerPC 64-bit instructions | expand

Commit Message

Richard Henderson April 30, 2021, 1:15 a.m. UTC
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/ppc/insn32.decode                   | 12 +++++++
 target/ppc/insn64.decode                   | 15 +++++++++
 target/ppc/translate.c                     | 29 ----------------
 target/ppc/translate/fixedpoint-impl.c.inc | 39 ++++++++++++++++++++++
 4 files changed, 66 insertions(+), 29 deletions(-)

-- 
2.25.1

Comments

Luis Fernando Fujita Pires April 30, 2021, 11:23 a.m. UTC | #1
From: Richard Henderson <richard.henderson@linaro.org>

> +&D              rt ra si

> +@D              ...... rt:5 ra:5 si:s16                 &D

> +

> +# If a prefix is allowed, decode with default values.

> +&PLS_D          rt ra si:int64_t r:bool

> +@PLS_D          ...... rt:5 ra:5 si:s16                 &PLS_D r=0

> +

> +### Fixed-Point Arithmetic Instructions

> +

> +ADDI            001110 ..... ..... ................     @PLS_D

> +ADDIS           001111 ..... ..... ................     @D

> diff --git a/target/ppc/insn64.decode b/target/ppc/insn64.decode index

> 9fc45d0614..f4272df724 100644

> --- a/target/ppc/insn64.decode

> +++ b/target/ppc/insn64.decode

> @@ -16,3 +16,18 @@

>  # You should have received a copy of the GNU Lesser General Public  # License

> along with this library; if not, see <http://www.gnu.org/licenses/>.

>  #

> +

> +# Many all of these instruction names would be prefixed by "P", # but

> +we share code with the non-prefixed instruction.

> +

> +# Format MLS:D and 8LS:D

> +&PLS_D          rt ra si:int64_t r:bool  !extern

> +%pls_si         32:s18 0:16

> +@PLS_D          ...... .. ... r:1 .. .................. \

> +                ...... rt:5 ra:5 ................       \

> +                &PLS_D si=%pls_si

> +

> +### Fixed-Point Arithmetic Instructions

> +

> +ADDI            000001 10 0--.-- ..................     \

> +                001110 ..... ..... ................     @PLS_D


I think we should reconsider using the same .decode file for both 32- and 64-bit instructions, to avoid duplicating argument set definitions, and to keep the prefixed instructions close to their non-prefixed counterparts. For ADDI/PADDI, something along the lines of:

&PLS_D          rt ra si:int64_t r:bool

%pls_si         32:s18 0:16
@PLS_D          ...... .. ... r:1 .. .................. \
                ...... rt:5 ra:5 ................       \
                &PLS_D si=%pls_si

@PLS_D_32       ...... rt:5 ra:5 si:s16                 &PLS_D r=0

PADDI           000001 10 0--.-- ..................     \
                001110 ..... ..... ................     @PLS_D
ADDI            001110 ..... ..... ................     @PLS_D_32
ADDIS           001111 ..... ..... ................     @D

That's where I was going with the original patch, using the varinsnwidth support from decodetree.py.

And, in order to share the trans_PADDI/ADDI implementation, maybe add something to decodetree.py to allow us to specify that an instruction shares the trans_XX() implementation from another one, such as:
ADDI            001110 ..... ..... ................     @PLS_D_32 !impl=PADDI

This way, we could (and would need to, in fact) keep the 'P' in the prefixed instruction names, but at the same time avoid having extra trans_XX functions just calling another one without any additional code.

For the load functions, we would then have:

%ds_si          2:s14  !function=times_4
@PLS_DS_32      ...... rt:5 ra:5 .............. ..      &PLS_D si=%ds_si r=0

&X              rt ra rb
@X              ...... rt:5 ra:5 rb:5 .......... .      &X

PLBZ            000001 10 0--.-- .................. \
                100010 ..... ..... ................     @PLS_D
LBZ             100010 ..... ..... ................     @PLS_D_32 !impl=PLBZ
LBZU            100011 ..... ..... ................     @PLS_D_32
LBZX            011111 ..... ..... ..... 0001010111 -   @X
LBZUX           011111 ..... ..... ..... 0001110111 -   @X

PLHZ            000001 10 0--.-- .................. \
                101000 ..... ..... ................     @PLS_D
LHZ             101000 ..... ..... ................     @PLS_D_32 !impl=PLHZ
LHZU            101001 ..... ..... ................     @PLS_D_32
LHZX            011111 ..... ..... ..... 0100010111 -   @X
LHZUX           011111 ..... ..... ..... 0100110111 -   @X

PLHA            000001 10 0--.-- .................. \
                101010 ..... ..... ................     @PLS_D
LHA             101010 ..... ..... ................     @PLS_D_32 !impl=PLHA
LHAU            101011 ..... ..... ................     @PLS_D_32
LHAX            011111 ..... ..... ..... 0101010111 -   @X
LHAXU           011111 ..... ..... ..... 0101110111 -   @X

PLWZ            000001 10 0--.-- .................. \
                100000 ..... ..... ................     @PLS_D
LWZ             100000 ..... ..... ................     @PLS_D_32 !impl=PLWZ
LWZU            100001 ..... ..... ................     @PLS_D_32
LWZX            011111 ..... ..... ..... 0000010111 -   @X
LWZUX           011111 ..... ..... ..... 0000110111 -   @X

PLWA            000001 00 0--.-- .................. \
                101001 ..... ..... ................     @PLS_D
LWA             111010 ..... ..... ..............10     @PLS_DS_32 !impl=PLWA
LWAX            011111 ..... ..... ..... 0101010101 -   @X
LWAUX           011111 ..... ..... ..... 0101110101 -   @X

PLD             000001 00 0--.-- .................. \
                111001 ..... ..... ................     @PLS_D
LD              111010 ..... ..... ..............00     @PLS_DS_32 !impl=PLD
LDU             111010 ..... ..... ..............01     @PLS_DS_32
LDX             011111 ..... ..... ..... 0000010101 -   @X
LDUX            011111 ..... ..... ..... 0000110101 -   @X

--
Luis
Matheus K. Ferst April 30, 2021, 2:05 p.m. UTC | #2
On 29/04/2021 22:15, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>   target/ppc/insn32.decode                   | 12 +++++++

>   target/ppc/insn64.decode                   | 15 +++++++++

>   target/ppc/translate.c                     | 29 ----------------

>   target/ppc/translate/fixedpoint-impl.c.inc | 39 ++++++++++++++++++++++

>   4 files changed, 66 insertions(+), 29 deletions(-)

> 

> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode

> index b175441209..52d9b355d4 100644

> --- a/target/ppc/insn32.decode

> +++ b/target/ppc/insn32.decode

> @@ -16,3 +16,15 @@

>   # You should have received a copy of the GNU Lesser General Public

>   # License along with this library; if not, see <http://www.gnu.org/licenses/>.

>   #

> +

> +&D              rt ra si

> +@D              ...... rt:5 ra:5 si:s16                 &D

> +

> +# If a prefix is allowed, decode with default values.

> +&PLS_D          rt ra si:int64_t r:bool

> +@PLS_D          ...... rt:5 ra:5 si:s16                 &PLS_D r=0

> +

> +### Fixed-Point Arithmetic Instructions

> +

> +ADDI            001110 ..... ..... ................     @PLS_D

> +ADDIS           001111 ..... ..... ................     @D

> diff --git a/target/ppc/insn64.decode b/target/ppc/insn64.decode

> index 9fc45d0614..f4272df724 100644

> --- a/target/ppc/insn64.decode

> +++ b/target/ppc/insn64.decode

> @@ -16,3 +16,18 @@

>   # You should have received a copy of the GNU Lesser General Public

>   # License along with this library; if not, see <http://www.gnu.org/licenses/>.

>   #

> +

> +# Many all of these instruction names would be prefixed by "P",

> +# but we share code with the non-prefixed instruction.

> +

> +# Format MLS:D and 8LS:D

> +&PLS_D          rt ra si:int64_t r:bool  !extern

> +%pls_si         32:s18 0:16

> +@PLS_D          ...... .. ... r:1 .. .................. \

> +                ...... rt:5 ra:5 ................       \

> +                &PLS_D si=%pls_si

> +

> +### Fixed-Point Arithmetic Instructions

> +

> +ADDI            000001 10 0--.-- ..................     \

> +                001110 ..... ..... ................     @PLS_D


I'm not sure about this. It's a bit surprising to find ADDI here, and 
the comment that explains why is likely to be ignored after the big 
copyright header.

<snip>

> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc

> index b740083605..7af1b3bcf5 100644

> --- a/target/ppc/translate/fixedpoint-impl.c.inc

> +++ b/target/ppc/translate/fixedpoint-impl.c.inc

> @@ -16,3 +16,42 @@

>    * You should have received a copy of the GNU Lesser General Public

>    * License along with this library; if not, see <http://www.gnu.org/licenses/>.

>    */

> +

> +/*

> + * Incorporate CIA into the constant when R=1.

> + * Validate that when R=1, RA=0.

> + */

> +static bool resolve_PLS_D(DisasContext *ctx, arg_PLS_D *a)

> +{

> +    if (a->r) {

> +        if (unlikely(a->ra != 0)) {

> +            gen_invalid(ctx);

> +            return false;

> +        }

> +        a->si += ctx->cia;

> +    }

> +    return true;

> +}

> +

> +static bool trans_ADDI(DisasContext *ctx, arg_PLS_D *a)

> +{

> +    if (resolve_PLS_D(ctx, a)) {

> +        if (a->ra) {

> +            tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], a->si);

> +        } else {

> +            tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);

> +        }

> +    }

> +    return true;

> +}

> +


I'd prefer to keep a trans_PADDI like

 > static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)

 > {

 >     if(!resolve_PLS_D(ctx, a)) {

 >         return false;

 >     }

 >     return trans_ADDI(ctx, a);

 > }


It's the middle way between v2 and v3. trans_ADDI code is reused, it'll 
probably be optimized as a tail call, and resolve_PLS_D is not called 
when it's not needed.

> +static bool trans_ADDIS(DisasContext *ctx, arg_D *a)

> +{

> +    int si = a->si << 16;

> +    if (a->ra) {

> +        tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], si);

> +    } else {

> +        tcg_gen_movi_tl(cpu_gpr[a->rt], si);

> +    }

> +    return true;

> +}

> 


I'd also keep this as in the last version, where trans_ADDI is called.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Richard Henderson April 30, 2021, 2:23 p.m. UTC | #3
On 4/30/21 4:23 AM, Luis Fernando Fujita Pires wrote:
> I think we should reconsider using the same .decode file for both 32- and

> 64-bit instructions, to avoid duplicating argument set definitions, and to

> keep the prefixed instructions close to their non-prefixed counterparts.

varinsnwidth assumes there is no easy way to determine, before decoding, the 
width of the instruction.  The way this is implemented in decodetree is vastly 
less optimal than what we can do with a few lines for ppc.

In addition, there's a rough spot in %field definitions.  You can't share those 
between patterns of different sizes, which can get confusing.  Have a look at 
target/rx, and the definitions of %b[23]_r_0, which is the same field for 2 and 
3-byte insns.

The replication of argument set definitions is unfortunate, but in the end will 
only be a handful of lines.  We could probably come up with a way to avoid that 
too, via a decodetree extension, if you really insist.  (My vague idea there 
would put the argument set definitions into a 3rd file, included on the 
decodetree command-line.)

> And, in order to share the trans_PADDI/ADDI implementation, maybe add something to decodetree.py to allow us to specify that an instruction shares the trans_XX() implementation from another one, such as:

> ADDI            001110 ..... ..... ................     @PLS_D_32 !impl=PADDI


This is done by using the same name up front.
If you like, add a comment to give the real instruction name.

PADDI   001110 ..... ..... ................     @PLS_D_32 # ADDI


> This way, we could (and would need to, in fact) keep the 'P' in the prefixed instruction names, but at the same time avoid having extra trans_XX functions just calling another one without any additional code.


I don't understand this at all.



r~
Richard Henderson April 30, 2021, 2:31 p.m. UTC | #4
On 4/30/21 7:05 AM, Matheus K. Ferst wrote:
>> +ADDI            000001 10 0--.-- ..................     \

>> +                001110 ..... ..... ................     @PLS_D

> 

> I'm not sure about this. It's a bit surprising to find ADDI here, and the 

> comment that explains why is likely to be ignored after the big copyright header.


You could move the comment closer, and replicate, e.g.

ADDI .... \
      .... @PLS_D # PADDI


> I'd prefer to keep a trans_PADDI like

> 

>  > static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)

>  > {

>  >     if(!resolve_PLS_D(ctx, a)) {

>  >         return false;

>  >     }

>  >     return trans_ADDI(ctx, a);

>  > }


But in this case ADDI probably doesn't use PLS_D.  You could use

static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)
{
     arg_D d;
     if (!resolve_PLS_D(ctx, &d, a)) {
         return false;
     }
     return trans_ADDI(ctx, &d);
}

making sure to use int64_t in the offset for arg_D.

> It's the middle way between v2 and v3. trans_ADDI code is reused, it'll 

> probably be optimized as a tail call, and resolve_PLS_D is not called when it's 

> not needed.


My version won't tail-call, because of the escaping local storage, but I don't 
see how you can avoid it.


r~
Matheus K. Ferst April 30, 2021, 6:02 p.m. UTC | #5
On 30/04/2021 11:31, Richard Henderson wrote:
> On 4/30/21 7:05 AM, Matheus K. Ferst wrote:

>>> +ADDI            000001 10 0--.-- ..................     \

>>> +                001110 ..... ..... ................     @PLS_D

>>

>> I'm not sure about this. It's a bit surprising to find ADDI here, and 

>> the comment that explains why is likely to be ignored after the big 

>> copyright header.

> 

> You could move the comment closer, and replicate, e.g.

> 

> ADDI .... \

>       .... @PLS_D # PADDI

> 

> 


If we keep this naming, IMHO moving the comment closer looks better.

>> I'd prefer to keep a trans_PADDI like

>>

>>  > static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)

>>  > {

>>  >     if(!resolve_PLS_D(ctx, a)) {

>>  >         return false;

>>  >     }

>>  >     return trans_ADDI(ctx, a);

>>  > }

> 

> But in this case ADDI probably doesn't use PLS_D.  You could use

> 

> static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)

> {

>      arg_D d;

>      if (!resolve_PLS_D(ctx, &d, a)) {

>          return false;

>      }

>      return trans_ADDI(ctx, &d);

> }

> 

> making sure to use int64_t in the offset for arg_D.

> 


We'd keep trans_ADDI with the same signature to avoid creating an arg_D 
on the stack. Patch 4 added type specification, maybe we can define an 
arg_D within arg_PLD_D? I'll play a bit to see if it works.

>> It's the middle way between v2 and v3. trans_ADDI code is reused, 

>> it'll probably be optimized as a tail call, and resolve_PLS_D is not 

>> called when it's not needed.

> 

> My version won't tail-call, because of the escaping local storage, but I 

> don't see how you can avoid it.

> 

> 

> r~


I haven't been able to test it properly yet, but at least on godbolt it 
seems that the compiler prefers to inline over tail call, so maybe 
that's not a problem.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Richard Henderson April 30, 2021, 6:43 p.m. UTC | #6
On 4/30/21 11:02 AM, Matheus K. Ferst wrote:
>> But in this case ADDI probably doesn't use PLS_D.  You could use

>>

>> static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)

>> {

>>      arg_D d;

>>      if (!resolve_PLS_D(ctx, &d, a)) {

>>          return false;

>>      }

>>      return trans_ADDI(ctx, &d);

>> }

>>

>> making sure to use int64_t in the offset for arg_D.

>>

> 

> We'd keep trans_ADDI with the same signature to avoid creating an arg_D on the 

> stack. Patch 4 added type specification, maybe we can define an arg_D within 

> arg_PLD_D? I'll play a bit to see if it works.


That starts to creep, with e.g. ADDIS now requiring arg_PLD_D.  You'll want to 
audit the other D-form insns to see what other special cases there are.

r~
Luis Fernando Fujita Pires April 30, 2021, 6:45 p.m. UTC | #7
From: Richard Henderson <richard.henderson@linaro.org>

> On 4/30/21 4:23 AM, Luis Fernando Fujita Pires wrote:

> > I think we should reconsider using the same .decode file for both 32-

> > and 64-bit instructions, to avoid duplicating argument set

> > definitions, and to keep the prefixed instructions close to their non-prefixed

> counterparts.

> varinsnwidth assumes there is no easy way to determine, before decoding, the

> width of the instruction.  The way this is implemented in decodetree is vastly less

> optimal than what we can do with a few lines for ppc.


I tried to solve this with one of the previous decodetree patches ("decodetree: Allow custom var width load functions"), whose goal was to allow us to implement a custom instruction load function (in reality, the only effect it had inside decodetree.py was to not generate the _load function).
So the instruction load would still be handled by a simple function inside translate.c, but we would use the auto-generated decode() function to call the trans_XX() functions.

> In addition, there's a rough spot in %field definitions.  You can't share those

> between patterns of different sizes, which can get confusing.  Have a look at

> target/rx, and the definitions of %b[23]_r_0, which is the same field for 2 and 3-

> byte insns.


Right. In the current patch we're already using separate definitions for 'si' depending on the format (%pls_si and %ds_si below):

&PLS_D          rt ra si:int64_t r:bool

%pls_si         32:s18 0:16
@PLS_D          ...... .. ... r:1 .. .................. \
                ...... rt:5 ra:5 ................       \
                &PLS_D si=%pls_si

@PLS_D_32       ...... rt:5 ra:5 si:s16                 &PLS_D r=0

%ds_si          2:s14  !function=times_4
@PLS_DS_32      ...... rt:5 ra:5 .............. ..      &PLS_D si=%ds_si r=0

And I also had to create separate @formats for 32- and 64-bit versions (@PLS_D, @PLS_D_32, etc.), which isn't that nice either.

> The replication of argument set definitions is unfortunate, but in the end will

> only be a handful of lines.  We could probably come up with a way to avoid that

> too, via a decodetree extension, if you really insist.  (My vague idea there would

> put the argument set definitions into a 3rd file, included on the decodetree

> command-line.)


I think we can already pass multiple files to decodetree.py and it will handle them correctly. I just didn't find a way to do that from the meson build files, which assume decodetree will always use a single input file.
Another option would be to allow files to be included from inside other .decode files.

> > And, in order to share the trans_PADDI/ADDI implementation, maybe add

> something to decodetree.py to allow us to specify that an instruction shares the

> trans_XX() implementation from another one, such as:

> > ADDI            001110 ..... ..... ................     @PLS_D_32 !impl=PADDI

> 

> This is done by using the same name up front.

> If you like, add a comment to give the real instruction name.

> 

> PADDI   001110 ..... ..... ................     @PLS_D_32 # ADDI

> 

> 

> > This way, we could (and would need to, in fact) keep the 'P' in the prefixed

> instruction names, but at the same time avoid having extra trans_XX functions

> just calling another one without any additional code.

> 

> I don't understand this at all.


Not a big deal. I was just referring to the fact that, in the current patch, you noted that the instruction names in insn64.decode were not prefixed by "P" due to the code sharing with the 32-bit instructions.

Thanks!
Luis
Richard Henderson April 30, 2021, 7:11 p.m. UTC | #8
On 4/30/21 11:45 AM, Luis Fernando Fujita Pires wrote:
> I think we can already pass multiple files to decodetree.py and it will handle them correctly. I just didn't find a way to do that from the meson build files, which assume decodetree will always use a single input file.


Oh, riscv does this via extra_args:.

r~
Luis Fernando Fujita Pires April 30, 2021, 8:32 p.m. UTC | #9
From: Richard Henderson <richard.henderson@linaro.org>

> On 4/30/21 11:45 AM, Luis Fernando Fujita Pires wrote:

> > I think we can already pass multiple files to decodetree.py and it will handle

> them correctly. I just didn't find a way to do that from the meson build files,

> which assume decodetree will always use a single input file.

> 

> Oh, riscv does this via extra_args:.

> 

> r~


The build system probably will fail to detect that a rebuild is needed if the file passed in through extra_args is changed though, right?
Richard Henderson April 30, 2021, 10:29 p.m. UTC | #10
On 4/30/21 1:32 PM, Luis Fernando Fujita Pires wrote:
> From: Richard Henderson <richard.henderson@linaro.org>

>> On 4/30/21 11:45 AM, Luis Fernando Fujita Pires wrote:

>>> I think we can already pass multiple files to decodetree.py and it will handle

>> them correctly. I just didn't find a way to do that from the meson build files,

>> which assume decodetree will always use a single input file.

>>

>> Oh, riscv does this via extra_args:.

>>

>> r~

> 

> The build system probably will fail to detect that a rebuild is needed if the file passed in through extra_args is changed though, right?


Oh, true.  Good thing there are patches for riscv to remove that.  :-P


r~
Matheus K. Ferst April 30, 2021, 11:29 p.m. UTC | #11
On 30/04/2021 15:43, Richard Henderson wrote:
> On 4/30/21 11:02 AM, Matheus K. Ferst wrote:

>>> But in this case ADDI probably doesn't use PLS_D.  You could use

>>> 

>>> static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a) { arg_D 

>>> d; if (!resolve_PLS_D(ctx, &d, a)) { return false; } return 

>>> trans_ADDI(ctx, &d); }

>>> 

>>> making sure to use int64_t in the offset for arg_D.

>>> 

>> 

>> We'd keep trans_ADDI with the same signature to avoid creating an 

>> arg_D on the stack. Patch 4 added type specification, maybe we can 

>> define an arg_D within arg_PLD_D? I'll play a bit to see if it 

>> works.

> 

> That starts to creep, with e.g. ADDIS now requiring arg_PLD_D. You'll

> want to audit the other D-form insns to see what other special cases

> there are.

> 

> r~


Well, anything that shares implementation with a prefixed instruction
would use the prefixed arg.

I got arg_D within arg_PLD_D using !function and calling
decode_insn32_extract_D. A bit on the ugly side, and probably not
worth the effort. Maybe changing decodetree would help, but that's 
another patch, for another series.

Anyway, my compiler (GCC 9.3) is inlining trans_ADDI calls with -O3, so 
I'd say that your trans_PADDI from the previous message, with trans_ADDI 
and trans_ADDIS using arg_D, would be the cleaner solution.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
diff mbox series

Patch

diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index b175441209..52d9b355d4 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -16,3 +16,15 @@ 
 # You should have received a copy of the GNU Lesser General Public
 # License along with this library; if not, see <http://www.gnu.org/licenses/>.
 #
+
+&D              rt ra si
+@D              ...... rt:5 ra:5 si:s16                 &D
+
+# If a prefix is allowed, decode with default values.
+&PLS_D          rt ra si:int64_t r:bool
+@PLS_D          ...... rt:5 ra:5 si:s16                 &PLS_D r=0
+
+### Fixed-Point Arithmetic Instructions
+
+ADDI            001110 ..... ..... ................     @PLS_D
+ADDIS           001111 ..... ..... ................     @D
diff --git a/target/ppc/insn64.decode b/target/ppc/insn64.decode
index 9fc45d0614..f4272df724 100644
--- a/target/ppc/insn64.decode
+++ b/target/ppc/insn64.decode
@@ -16,3 +16,18 @@ 
 # You should have received a copy of the GNU Lesser General Public
 # License along with this library; if not, see <http://www.gnu.org/licenses/>.
 #
+
+# Many all of these instruction names would be prefixed by "P",
+# but we share code with the non-prefixed instruction.
+
+# Format MLS:D and 8LS:D
+&PLS_D          rt ra si:int64_t r:bool  !extern
+%pls_si         32:s18 0:16
+@PLS_D          ...... .. ... r:1 .. .................. \
+                ...... rt:5 ra:5 ................       \
+                &PLS_D si=%pls_si
+
+### Fixed-Point Arithmetic Instructions
+
+ADDI            000001 10 0--.-- ..................     \
+                001110 ..... ..... ................     @PLS_D
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index d782a13d27..5a8a3c39c3 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -924,19 +924,6 @@  GEN_INT_ARITH_ADD(addex, 0x05, cpu_ov, 1, 1, 0);
 /* addze  addze.  addzeo  addzeo.*/
 GEN_INT_ARITH_ADD_CONST(addze, 0x06, 0, cpu_ca, 1, 1, 0)
 GEN_INT_ARITH_ADD_CONST(addzeo, 0x16, 0, cpu_ca, 1, 1, 1)
-/* addi */
-static void gen_addi(DisasContext *ctx)
-{
-    target_long simm = SIMM(ctx->opcode);
-
-    if (rA(ctx->opcode) == 0) {
-        /* li case */
-        tcg_gen_movi_tl(cpu_gpr[rD(ctx->opcode)], simm);
-    } else {
-        tcg_gen_addi_tl(cpu_gpr[rD(ctx->opcode)],
-                        cpu_gpr[rA(ctx->opcode)], simm);
-    }
-}
 /* addic  addic.*/
 static inline void gen_op_addic(DisasContext *ctx, bool compute_rc0)
 {
@@ -956,20 +943,6 @@  static void gen_addic_(DisasContext *ctx)
     gen_op_addic(ctx, 1);
 }
 
-/* addis */
-static void gen_addis(DisasContext *ctx)
-{
-    target_long simm = SIMM(ctx->opcode);
-
-    if (rA(ctx->opcode) == 0) {
-        /* lis case */
-        tcg_gen_movi_tl(cpu_gpr[rD(ctx->opcode)], simm << 16);
-    } else {
-        tcg_gen_addi_tl(cpu_gpr[rD(ctx->opcode)],
-                        cpu_gpr[rA(ctx->opcode)], simm << 16);
-    }
-}
-
 /* addpcis */
 static void gen_addpcis(DisasContext *ctx)
 {
@@ -7029,10 +7002,8 @@  GEN_HANDLER_E(cmpeqb, 0x1F, 0x00, 0x07, 0x00600000, PPC_NONE, PPC2_ISA300),
 GEN_HANDLER_E(cmpb, 0x1F, 0x1C, 0x0F, 0x00000001, PPC_NONE, PPC2_ISA205),
 GEN_HANDLER_E(cmprb, 0x1F, 0x00, 0x06, 0x00400001, PPC_NONE, PPC2_ISA300),
 GEN_HANDLER(isel, 0x1F, 0x0F, 0xFF, 0x00000001, PPC_ISEL),
-GEN_HANDLER(addi, 0x0E, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
 GEN_HANDLER(addic, 0x0C, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
 GEN_HANDLER2(addic_, "addic.", 0x0D, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
-GEN_HANDLER(addis, 0x0F, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
 GEN_HANDLER_E(addpcis, 0x13, 0x2, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300),
 GEN_HANDLER(mulhw, 0x1F, 0x0B, 0x02, 0x00000400, PPC_INTEGER),
 GEN_HANDLER(mulhwu, 0x1F, 0x0B, 0x00, 0x00000400, PPC_INTEGER),
diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
index b740083605..7af1b3bcf5 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -16,3 +16,42 @@ 
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+
+/*
+ * Incorporate CIA into the constant when R=1.
+ * Validate that when R=1, RA=0.
+ */
+static bool resolve_PLS_D(DisasContext *ctx, arg_PLS_D *a)
+{
+    if (a->r) {
+        if (unlikely(a->ra != 0)) {
+            gen_invalid(ctx);
+            return false;
+        }
+        a->si += ctx->cia;
+    }
+    return true;
+}
+
+static bool trans_ADDI(DisasContext *ctx, arg_PLS_D *a)
+{
+    if (resolve_PLS_D(ctx, a)) {
+        if (a->ra) {
+            tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], a->si);
+        } else {
+            tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);
+        }
+    }
+    return true;
+}
+
+static bool trans_ADDIS(DisasContext *ctx, arg_D *a)
+{
+    int si = a->si << 16;
+    if (a->ra) {
+        tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], si);
+    } else {
+        tcg_gen_movi_tl(cpu_gpr[a->rt], si);
+    }
+    return true;
+}