Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Michael Rolnik]

Sounds good, thanks!

On Tue, Nov 19, 2019 at 11:18 PM Aleksandar Markovic
 wrote:
>
> On Tue, Nov 19, 2019 at 9:09 PM Michael Rolnik  wrote:
> >
> > Hi Aleksandar et al.
> >
> > how is it going? should I rebase or not?
> >
> > Michael
> >
>
> I am just in the process of taking a detailed look to the whole series.
>
> I don't have any serious objection so far, I may just suggest a couple
> of things that will improve readability of the code - so nothing of some
> really serious nature.
>
> Stay tuned (and be patient). Will hear from me soon.
>
> Yours, Aleksandar



-- 
Best Regards,
Michael Rolnik

Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Aleksandar Markovic]

On Tue, Nov 19, 2019 at 9:09 PM Michael Rolnik  wrote:
>
> Hi Aleksandar et al.
>
> how is it going? should I rebase or not?
>
> Michael
>

I am just in the process of taking a detailed look to the whole series.

I don't have any serious objection so far, I may just suggest a couple
of things that will improve readability of the code - so nothing of some
really serious nature.

Stay tuned (and be patient). Will hear from me soon.

Yours, Aleksandar

Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Michael Rolnik]

Hi Aleksandar et al.

how is it going? should I rebase or not?

Michael

On Tue, Nov 5, 2019 at 4:38 PM Aleksandar Markovic
 wrote:
>
> On Tue, Nov 5, 2019 at 2:23 PM Richard Henderson
>  wrote:
> >
> > On 11/5/19 10:46 AM, Aleksandar Markovic wrote:
> > >
> > >
> > > On Tuesday, November 5, 2019, Aleksandar Markovic 
> > >  > > > wrote:
> > >
> > >
> > > +
> > > +/*
> > > + *  This instruction performs 8-bit x 8-bit -> 16-bit signed
> > > multiplication
> > > + *  and shifts the result one bit left.
> > > + */
> > > +static bool trans_FMULSU(DisasContext *ctx, arg_FMULSU *a)
> > > +{
> > > +if (!avr_have_feature(ctx, AVR_FEATURE_MUL)) {
> > > +return true;
> > > +}
> > > +
> > > +TCGv R0 = cpu_r[0];
> > > +TCGv R1 = cpu_r[1];
> > > +TCGv Rd = cpu_r[a->rd];
> > > +TCGv Rr = cpu_r[a->rr];
> > > +TCGv R = tcg_temp_new_i32();
> > > +TCGv t0 = tcg_temp_new_i32();
> > > +
> > > +tcg_gen_ext8s_tl(t0, Rd); /* make Rd full 32 bit signed */
> > > +tcg_gen_mul_tl(R, t0, Rr); /* R = Rd * Rr */
> > > +tcg_gen_andi_tl(R, R, 0x); /* make it 16 bits */
> > > +
> > > +tcg_gen_shri_tl(cpu_Cf, R, 15); /* Cf = R(15) */
> > > +tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 
> > > 0 */
> > > +
> > > +tcg_gen_shli_tl(R, R, 1);
> > > +
> > > +tcg_gen_andi_tl(R0, R, 0xff);
> > > +tcg_gen_shri_tl(R1, R, 8);
> > > +tcg_gen_andi_tl(R1, R1, 0xff);
> > > +
> > > +tcg_temp_free_i32(t0);
> > > +tcg_temp_free_i32(R);
> > > +
> > > +return true;
> > > +}
> > > +
> > >
> > >
> > > Hi, Michael.
> > >
> > > The way I understand the spec is that a->rd and a->rd must be between 
> > > 16
> > > and 23:
> > >
> > > 
> > > https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_FMULSU.html
> > > 
> > > 
> > >
> > > Is my interpretation right? If yes, where is the corresponding part 
> > > in the
> > > implementation?
> > >
> > >
> > > Or, perhaps,
> > >
> > > TCGv Rd = cpu_r[a->rd];
> > >
> > > should be
> > >
> > > TCGv Rd = cpu_r[a->rd + 16];
> > >
> > > (and the same for rs)
> >
> > This happens during decode:
> >
> > +%rd_b   4:3 !function=to_B
> > +%rr_b   0:3 !function=to_B
> > +@fmul     . ... . ...   _rr  rd=%rd_b rr=%rr_b
> > +FMUL 0011 0 ... 1 ...   @fmul
> > +FMULS    0011 1 ... 0 ...   @fmul
> > +FMULSU   0011 1 ... 1 ...   @fmul
> >
> > This means that a->rd = to_B(extract32(insn, 4, 3)), and
> >
> > > +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
> >
> > et voila.
> >
>
> OK. Thanks for clarification.
>
> >
> > r~



-- 
Best Regards,
Michael Rolnik

Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Aleksandar Markovic]

On Tue, Nov 5, 2019 at 2:23 PM Richard Henderson
 wrote:
>
> On 11/5/19 10:46 AM, Aleksandar Markovic wrote:
> >
> >
> > On Tuesday, November 5, 2019, Aleksandar Markovic 
> >  > > wrote:
> >
> >
> > +
> > +/*
> > + *  This instruction performs 8-bit x 8-bit -> 16-bit signed
> > multiplication
> > + *  and shifts the result one bit left.
> > + */
> > +static bool trans_FMULSU(DisasContext *ctx, arg_FMULSU *a)
> > +{
> > +if (!avr_have_feature(ctx, AVR_FEATURE_MUL)) {
> > +return true;
> > +}
> > +
> > +TCGv R0 = cpu_r[0];
> > +TCGv R1 = cpu_r[1];
> > +TCGv Rd = cpu_r[a->rd];
> > +TCGv Rr = cpu_r[a->rr];
> > +TCGv R = tcg_temp_new_i32();
> > +TCGv t0 = tcg_temp_new_i32();
> > +
> > +tcg_gen_ext8s_tl(t0, Rd); /* make Rd full 32 bit signed */
> > +tcg_gen_mul_tl(R, t0, Rr); /* R = Rd * Rr */
> > +tcg_gen_andi_tl(R, R, 0x); /* make it 16 bits */
> > +
> > +tcg_gen_shri_tl(cpu_Cf, R, 15); /* Cf = R(15) */
> > +tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 0 
> > */
> > +
> > +tcg_gen_shli_tl(R, R, 1);
> > +
> > +tcg_gen_andi_tl(R0, R, 0xff);
> > +tcg_gen_shri_tl(R1, R, 8);
> > +tcg_gen_andi_tl(R1, R1, 0xff);
> > +
> > +tcg_temp_free_i32(t0);
> > +tcg_temp_free_i32(R);
> > +
> > +return true;
> > +}
> > +
> >
> >
> > Hi, Michael.
> >
> > The way I understand the spec is that a->rd and a->rd must be between 16
> > and 23:
> >
> > 
> > https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_FMULSU.html
> > 
> > 
> >
> > Is my interpretation right? If yes, where is the corresponding part in 
> > the
> > implementation?
> >
> >
> > Or, perhaps,
> >
> > TCGv Rd = cpu_r[a->rd];
> >
> > should be
> >
> > TCGv Rd = cpu_r[a->rd + 16];
> >
> > (and the same for rs)
>
> This happens during decode:
>
> +%rd_b   4:3 !function=to_B
> +%rr_b   0:3 !function=to_B
> +@fmul     . ... . ...   _rr  rd=%rd_b rr=%rr_b
> +FMUL 0011 0 ... 1 ...   @fmul
> +FMULS    0011 1 ... 0 ...   @fmul
> +FMULSU   0011 1 ... 1 ...   @fmul
>
> This means that a->rd = to_B(extract32(insn, 4, 3)), and
>
> > +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }
>
> et voila.
>

OK. Thanks for clarification.

>
> r~

Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Richard Henderson]

On 11/5/19 10:46 AM, Aleksandar Markovic wrote:
> 
> 
> On Tuesday, November 5, 2019, Aleksandar Markovic  > wrote:
> 
> 
> +
> +/*
> + *  This instruction performs 8-bit x 8-bit -> 16-bit signed
> multiplication
> + *  and shifts the result one bit left.
> + */
> +static bool trans_FMULSU(DisasContext *ctx, arg_FMULSU *a)
> +{
> +    if (!avr_have_feature(ctx, AVR_FEATURE_MUL)) {
> +        return true;
> +    }
> +
> +    TCGv R0 = cpu_r[0];
> +    TCGv R1 = cpu_r[1];
> +    TCGv Rd = cpu_r[a->rd];
> +    TCGv Rr = cpu_r[a->rr];
> +    TCGv R = tcg_temp_new_i32();
> +    TCGv t0 = tcg_temp_new_i32();
> +
> +    tcg_gen_ext8s_tl(t0, Rd); /* make Rd full 32 bit signed */
> +    tcg_gen_mul_tl(R, t0, Rr); /* R = Rd * Rr */
> +    tcg_gen_andi_tl(R, R, 0x); /* make it 16 bits */
> +
> +    tcg_gen_shri_tl(cpu_Cf, R, 15); /* Cf = R(15) */
> +    tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 0 */
> +
> +    tcg_gen_shli_tl(R, R, 1);
> +
> +    tcg_gen_andi_tl(R0, R, 0xff);
> +    tcg_gen_shri_tl(R1, R, 8);
> +    tcg_gen_andi_tl(R1, R1, 0xff);
> +
> +    tcg_temp_free_i32(t0);
> +    tcg_temp_free_i32(R);
> +
> +    return true;
> +}
> +
> 
> 
> Hi, Michael.
> 
> The way I understand the spec is that a->rd and a->rd must be between 16
> and 23:
> 
> https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_FMULSU.html
> 
> 
> 
> Is my interpretation right? If yes, where is the corresponding part in the
> implementation?
> 
> 
> Or, perhaps,
> 
> TCGv Rd = cpu_r[a->rd];
> 
> should be
> 
> TCGv Rd = cpu_r[a->rd + 16];
>  
> (and the same for rs)

This happens during decode:

+%rd_b   4:3 !function=to_B
+%rr_b   0:3 !function=to_B
+@fmul     . ... . ...   _rr  rd=%rd_b rr=%rr_b
+FMUL 0011 0 ... 1 ...   @fmul
+FMULS    0011 1 ... 0 ...   @fmul
+FMULSU   0011 1 ... 1 ...   @fmul

This means that a->rd = to_B(extract32(insn, 4, 3)), and

> +static int to_B(DisasContext *ctx, int indx) { return 16 + (indx % 8); }

et voila.


r~

Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Aleksandar Markovic]

On Tuesday, November 5, 2019, Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>> +
>> +/*
>> + *  This instruction performs 8-bit x 8-bit -> 16-bit signed
>> multiplication
>> + *  and shifts the result one bit left.
>> + */
>> +static bool trans_FMULSU(DisasContext *ctx, arg_FMULSU *a)
>> +{
>> +if (!avr_have_feature(ctx, AVR_FEATURE_MUL)) {
>> +return true;
>> +}
>> +
>> +TCGv R0 = cpu_r[0];
>> +TCGv R1 = cpu_r[1];
>> +TCGv Rd = cpu_r[a->rd];
>> +TCGv Rr = cpu_r[a->rr];
>> +TCGv R = tcg_temp_new_i32();
>> +TCGv t0 = tcg_temp_new_i32();
>> +
>> +tcg_gen_ext8s_tl(t0, Rd); /* make Rd full 32 bit signed */
>> +tcg_gen_mul_tl(R, t0, Rr); /* R = Rd * Rr */
>> +tcg_gen_andi_tl(R, R, 0x); /* make it 16 bits */
>> +
>> +tcg_gen_shri_tl(cpu_Cf, R, 15); /* Cf = R(15) */
>> +tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 0 */
>> +
>> +tcg_gen_shli_tl(R, R, 1);
>> +
>> +tcg_gen_andi_tl(R0, R, 0xff);
>> +tcg_gen_shri_tl(R1, R, 8);
>> +tcg_gen_andi_tl(R1, R1, 0xff);
>> +
>> +tcg_temp_free_i32(t0);
>> +tcg_temp_free_i32(R);
>> +
>> +return true;
>> +}
>> +
>
>
> Hi, Michael.
>
> The way I understand the spec is that a->rd and a->rd must be between 16
> and 23:
>
> https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_FMULSU.html
>
> Is my interpretation right? If yes, where is the corresponding part in the
> implementation?
>
>
Or, perhaps,

TCGv Rd = cpu_r[a->rd];

should be

TCGv Rd = cpu_r[a->rd + 16];

(and the same for rs)

?


Yours, Aleksandar
>
>
>>

Re: [PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Aleksandar Markovic]

>
>
> +
> +/*
> + *  This instruction performs 8-bit x 8-bit -> 16-bit signed
> multiplication
> + *  and shifts the result one bit left.
> + */
> +static bool trans_FMULSU(DisasContext *ctx, arg_FMULSU *a)
> +{
> +if (!avr_have_feature(ctx, AVR_FEATURE_MUL)) {
> +return true;
> +}
> +
> +TCGv R0 = cpu_r[0];
> +TCGv R1 = cpu_r[1];
> +TCGv Rd = cpu_r[a->rd];
> +TCGv Rr = cpu_r[a->rr];
> +TCGv R = tcg_temp_new_i32();
> +TCGv t0 = tcg_temp_new_i32();
> +
> +tcg_gen_ext8s_tl(t0, Rd); /* make Rd full 32 bit signed */
> +tcg_gen_mul_tl(R, t0, Rr); /* R = Rd * Rr */
> +tcg_gen_andi_tl(R, R, 0x); /* make it 16 bits */
> +
> +tcg_gen_shri_tl(cpu_Cf, R, 15); /* Cf = R(15) */
> +tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 0 */
> +
> +tcg_gen_shli_tl(R, R, 1);
> +
> +tcg_gen_andi_tl(R0, R, 0xff);
> +tcg_gen_shri_tl(R1, R, 8);
> +tcg_gen_andi_tl(R1, R1, 0xff);
> +
> +tcg_temp_free_i32(t0);
> +tcg_temp_free_i32(R);
> +
> +return true;
> +}
> +


Hi, Michael.

The way I understand the spec is that a->rd and a->rd must be between 16
and 23:

https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_FMULSU.html

Is my interpretation right? If yes, where is the corresponding part in the
implementation?

Yours, Aleksandar


>

[PATCH v35 05/13] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Michael Rolnik]

This includes:
- ADD, ADC, ADIW
- SBIW, SUB, SUBI, SBC, SBCI
- AND, ANDI
- OR, ORI, EOR
- COM, NEG
- INC, DEC
- MUL, MULS, MULSU
- FMUL, FMULS, FMULSU
- DES

Signed-off-by: Michael Rolnik 
---
 target/avr/translate.c | 822 +
 1 file changed, 822 insertions(+)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 53c9892a60..573c9988b5 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -130,3 +130,825 @@ static bool avr_have_feature(DisasContext *ctx, int 
feature)
 static bool decode_insn(DisasContext *ctx, uint16_t insn);
 #include "decode_insn.inc.c"
 
+
+static void gen_add_CHf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+TCGv t3 = tcg_temp_new_i32();
+
+tcg_gen_and_tl(t1, Rd, Rr); /* t1 = Rd & Rr */
+tcg_gen_andc_tl(t2, Rd, R); /* t2 = Rd & ~R */
+tcg_gen_andc_tl(t3, Rr, R); /* t3 = Rr & ~R */
+tcg_gen_or_tl(t1, t1, t2); /* t1 = t1 | t2 | t3 */
+tcg_gen_or_tl(t1, t1, t3);
+
+tcg_gen_shri_tl(cpu_Cf, t1, 7); /* Cf = t1(7) */
+tcg_gen_shri_tl(cpu_Hf, t1, 3); /* Hf = t1(3) */
+tcg_gen_andi_tl(cpu_Hf, cpu_Hf, 1);
+
+tcg_temp_free_i32(t3);
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+
+static void gen_add_Vf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+
+/* t1 = Rd & Rr & ~R | ~Rd & ~Rr & R = (Rd ^ R) & ~(Rd ^ Rr) */
+tcg_gen_xor_tl(t1, Rd, R);
+tcg_gen_xor_tl(t2, Rd, Rr);
+tcg_gen_andc_tl(t1, t1, t2);
+
+tcg_gen_shri_tl(cpu_Vf, t1, 7); /* Vf = t1(7) */
+
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+
+static void gen_sub_CHf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+TCGv t3 = tcg_temp_new_i32();
+
+/* Cf & Hf */
+tcg_gen_not_tl(t1, Rd); /* t1 = ~Rd */
+tcg_gen_and_tl(t2, t1, Rr); /* t2 = ~Rd & Rr */
+tcg_gen_or_tl(t3, t1, Rr); /* t3 = (~Rd | Rr) & R */
+tcg_gen_and_tl(t3, t3, R);
+tcg_gen_or_tl(t2, t2, t3); /* t2 = ~Rd & Rr | ~Rd & R | R & Rr */
+tcg_gen_shri_tl(cpu_Cf, t2, 7); /* Cf = t2(7) */
+tcg_gen_shri_tl(cpu_Hf, t2, 3); /* Hf = t2(3) */
+tcg_gen_andi_tl(cpu_Hf, cpu_Hf, 1);
+
+tcg_temp_free_i32(t3);
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+
+static void gen_sub_Vf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+
+/* Vf */
+/* t1 = Rd & ~Rr & ~R | ~Rd & Rr & R = (Rd ^ R) & (Rd ^ R) */
+tcg_gen_xor_tl(t1, Rd, R);
+tcg_gen_xor_tl(t2, Rd, Rr);
+tcg_gen_and_tl(t1, t1, t2);
+tcg_gen_shri_tl(cpu_Vf, t1, 7); /* Vf = t1(7) */
+
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+
+static void gen_NSf(TCGv R)
+{
+tcg_gen_shri_tl(cpu_Nf, R, 7); /* Nf = R(7) */
+tcg_gen_xor_tl(cpu_Sf, cpu_Nf, cpu_Vf); /* Sf = Nf ^ Vf */
+}
+
+
+static void gen_ZNSf(TCGv R)
+{
+tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 0 */
+tcg_gen_shri_tl(cpu_Nf, R, 7); /* Nf = R(7) */
+tcg_gen_xor_tl(cpu_Sf, cpu_Nf, cpu_Vf); /* Sf = Nf ^ Vf */
+}
+
+
+/*
+ *  Adds two registers without the C Flag and places the result in the
+ *  destination register Rd.
+ */
+static bool trans_ADD(DisasContext *ctx, arg_ADD *a)
+{
+TCGv Rd = cpu_r[a->rd];
+TCGv Rr = cpu_r[a->rr];
+TCGv R = tcg_temp_new_i32();
+
+/* op */
+tcg_gen_add_tl(R, Rd, Rr); /* Rd = Rd + Rr */
+tcg_gen_andi_tl(R, R, 0xff); /* make it 8 bits */
+
+gen_add_CHf(R, Rd, Rr);
+gen_add_Vf(R, Rd, Rr);
+gen_ZNSf(R);
+
+/* R */
+tcg_gen_mov_tl(Rd, R);
+
+tcg_temp_free_i32(R);
+
+return true;
+}
+
+
+/*
+ *  Adds two registers and the contents of the C Flag and places the result in
+ *  the destination register Rd.
+ */
+static bool trans_ADC(DisasContext *ctx, arg_ADC *a)
+{
+TCGv Rd = cpu_r[a->rd];
+TCGv Rr = cpu_r[a->rr];
+TCGv R = tcg_temp_new_i32();
+
+/* op */
+tcg_gen_add_tl(R, Rd, Rr); /* R = Rd + Rr + Cf */
+tcg_gen_add_tl(R, R, cpu_Cf);
+tcg_gen_andi_tl(R, R, 0xff); /* make it 8 bits */
+
+gen_add_CHf(R, Rd, Rr);
+gen_add_Vf(R, Rd, Rr);
+gen_ZNSf(R);
+
+/* R */
+tcg_gen_mov_tl(Rd, R);
+
+tcg_temp_free_i32(R);
+
+return true;
+}
+
+
+/*
+ *  Subtracts an immediate value (0-63) from a register pair and places the
+ *  result in the register pair. This instruction operates on the upper four
+ *  register pairs, and is well suited for operations on the Pointer Registers.
+ *  This instruction is not available in all devices. Refer to the device
+ *  specific instruction set summary.
+ */
+static bool trans_SBIW(DisasContext *ctx, arg_SBIW *a)
+{
+if (!avr_have_feature(ctx, AVR_FEATURE_ADIW_SBIW)) {
+return true;
+}
+
+TCGv RdL = cpu_r[a->rd];
+TCGv RdH = cpu_r[a->rd + 1];
+int Imm = (a->imm);
+TCGv R = tcg_temp_new_i32();
+TCGv Rd =