[Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---

I've started a glibc test run with this, but I don't expect overmuch.
The only applications I can see are "bfffo *,0,32,dN" which isn't
exactly exhaustive.  Probably better to hand craft some tests vs real
hardware.

Considering the prevelance of small helper functions calling ctz/clz,
I'm thinking of adding an opcode to tcg for this.  Certainly all of
the common hosts support it natively...


r~
---
 target-m68k/helper.h    |  3 +++
 target-m68k/op_helper.c | 21 +++++++++++++++++++++
 target-m68k/translate.c | 39 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/target-m68k/helper.h b/target-m68k/helper.h
index c1a4818..a0fedde 100644
--- a/target-m68k/helper.h
+++ b/target-m68k/helper.h
@@ -77,9 +77,12 @@ DEF_HELPER_2(set_ccr, void, env, i32)
 DEF_HELPER_FLAGS_1(get_ccr, TCG_CALL_NO_WG_SE, i32, env)
 DEF_HELPER_2(raise_exception, void, env, i32)
 
+DEF_HELPER_FLAGS_3(bfffo_reg, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32)
+
 DEF_HELPER_FLAGS_4(bfexts_mem, TCG_CALL_NO_WG, i32, env, i32, s32, i32)
 DEF_HELPER_FLAGS_4(bfextu_mem, TCG_CALL_NO_WG, i64, env, i32, s32, i32)
 DEF_HELPER_FLAGS_5(bfins_mem, TCG_CALL_NO_WG, i32, env, i32, i32, s32, i32)
 DEF_HELPER_FLAGS_4(bfchg_mem, TCG_CALL_NO_WG, i32, env, i32, s32, i32)
 DEF_HELPER_FLAGS_4(bfclr_mem, TCG_CALL_NO_WG, i32, env, i32, s32, i32)
 DEF_HELPER_FLAGS_4(bfset_mem, TCG_CALL_NO_WG, i32, env, i32, s32, i32)
+DEF_HELPER_FLAGS_4(bfffo_mem, TCG_CALL_NO_WG, i64, env, i32, s32, i32)
diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
index a800033..12a8264 100644
--- a/target-m68k/op_helper.c
+++ b/target-m68k/op_helper.c
@@ -653,3 +653,24 @@ uint32_t HELPER(bfset_mem)(CPUM68KState *env, uint32_t addr,
 
     return ((data & mask) << d.bofs) >> 32;
 }
+
+uint32_t HELPER(bfffo_reg)(uint32_t n, uint32_t ofs, uint32_t len)
+{
+    return (n ? clz32(n) : len) + ofs;
+}
+
+uint64_t HELPER(bfffo_mem)(CPUM68KState *env, uint32_t addr,
+                           int32_t ofs, uint32_t len)
+{
+    uintptr_t ra = GETPC();
+    struct bf_data d = bf_prep(addr, ofs, len);
+    uint64_t data = bf_load(env, d.addr, d.blen, ra);
+    uint64_t mask = -1ull << (64 - d.len) >> d.bofs;
+    uint64_t n = (data & mask) << d.bofs;
+    uint32_t ffo = helper_bfffo_reg(n >> 32, ofs, d.len);
+
+    /* Return FFO in the low word and N in the high word.
+       Note that because of MASK and the shift, the low word
+       is already zero.  */
+    return n | ffo;
+}
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index bc6ec39..ed40ade 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3913,7 +3913,14 @@ DISAS_INSN(bfop_reg)
     TCGv src = DREG(insn, 0);
     int len = ((extract32(ext, 0, 5) - 1) & 31) + 1;
     int ofs = extract32(ext, 6, 5);  /* big bit-endian */
-    TCGv mask;
+    TCGv mask, tofs, tlen;
+
+    TCGV_UNUSED(tofs);
+    TCGV_UNUSED(tlen);
+    if ((insn & 0x0f00) == 0x0d00) { /* bfffo */
+        tofs = tcg_temp_new();
+        tlen = tcg_temp_new();
+    }
 
     if ((ext & 0x820) == 0) {
         /* Immediate width and offset.  */
@@ -3925,6 +3932,10 @@ DISAS_INSN(bfop_reg)
         }
         tcg_gen_andi_i32(QREG_CC_N, QREG_CC_N, ~maski);
         mask = tcg_const_i32(ror32(maski, ofs));
+        if (!TCGV_IS_UNUSED(tofs)) {
+            tcg_gen_movi_i32(tofs, ofs);
+            tcg_gen_movi_i32(tlen, len);
+        }
     } else {
         TCGv tmp = tcg_temp_new();
         if (ext & 0x20) {
@@ -3933,9 +3944,15 @@ DISAS_INSN(bfop_reg)
             tcg_gen_andi_i32(tmp, tmp, 31);
             mask = tcg_const_i32(0x7fffffffu);
             tcg_gen_shr_i32(mask, mask, tmp);
+            if (!TCGV_IS_UNUSED(tlen)) {
+                tcg_gen_mov_i32(tlen, tmp);
+            }
         } else {
             /* Immediate width */
             mask = tcg_const_i32(0x7fffffffu >> (len - 1));
+            if (!TCGV_IS_UNUSED(tlen)) {
+                tcg_gen_movi_i32(tlen, len);
+            }
         }
         if (ext & 0x800) {
             /* Variable offset */
@@ -3943,11 +3960,17 @@ DISAS_INSN(bfop_reg)
             tcg_gen_rotl_i32(QREG_CC_N, src, tmp);
             tcg_gen_andc_i32(QREG_CC_N, QREG_CC_N, mask);
             tcg_gen_rotr_i32(mask, mask, tmp);
+            if (!TCGV_IS_UNUSED(tofs)) {
+                tcg_gen_mov_tl(tofs, tmp);
+            }
         } else {
             /* Immediate offset (and variable width) */
             tcg_gen_rotli_i32(QREG_CC_N, src, ofs);
             tcg_gen_andc_i32(QREG_CC_N, QREG_CC_N, mask);
             tcg_gen_rotri_i32(mask, mask, ofs);
+            if (!TCGV_IS_UNUSED(tofs)) {
+                tcg_gen_movi_i32(tofs, ofs);
+            }
         }
         tcg_temp_free(tmp);
     }
@@ -3960,6 +3983,11 @@ DISAS_INSN(bfop_reg)
     case 0x0c00: /* bfclr */
         tcg_gen_and_i32(src, src, mask);
         break;
+    case 0x0d00: /* bfffo */
+        gen_helper_bfffo_reg(DREG(ext, 12), QREG_CC_N, tofs, tlen);
+        tcg_temp_free(tlen);
+        tcg_temp_free(tofs);
+        break;
     case 0x0e00: /* bfset */
         tcg_gen_orc_i32(src, src, mask);
         break;
@@ -3976,6 +4004,7 @@ DISAS_INSN(bfop_mem)
 {
     int ext = read_im16(env, s);
     TCGv addr, len, ofs;
+    TCGv_i64 t64;
 
     addr = gen_lea(env, s, insn, OS_UNSIZED);
     if (IS_NULL_QREG(addr)) {
@@ -4001,6 +4030,12 @@ DISAS_INSN(bfop_mem)
     case 0x0c00: /* bfclr */
         gen_helper_bfclr_mem(QREG_CC_N, cpu_env, addr, ofs, len);
         break;
+    case 0x0d00: /* bfffo */
+        t64 = tcg_temp_new_i64();
+        gen_helper_bfffo_mem(t64, cpu_env, addr, ofs, len);
+        tcg_gen_extr_i64_i32(DREG(ext, 12), QREG_CC_N, t64);
+        tcg_temp_free_i64(t64);
+        break;
     case 0x0e00: /* bfset */
         gen_helper_bfset_mem(QREG_CC_N, cpu_env, addr, ofs, len);
         break;
@@ -5415,6 +5450,8 @@ void register_m68k_insns (CPUM68KState *env)
     INSN(bfop_reg, eac0, fff8, BITFIELD);   /* bfchg */
     INSN(bfop_mem, ecc0, ffc0, BITFIELD);   /* bfclr */
     INSN(bfop_reg, ecc0, fff8, BITFIELD);   /* bfclr */
+    INSN(bfop_mem, edc0, ffc0, BITFIELD);   /* bfffo */
+    INSN(bfop_reg, edc0, fff8, BITFIELD);   /* bfffo */
     INSN(bfop_mem, eec0, ffc0, BITFIELD);   /* bfset */
     INSN(bfop_reg, eec0, fff8, BITFIELD);   /* bfset */
     INSN(bfop_mem, e8c0, ffc0, BITFIELD);   /* bftst */
-- 
2.7.4

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Laurent Vivier]

Le 15/11/2016 à 21:44, Richard Henderson a écrit :
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
> 
> I've started a glibc test run with this, but I don't expect overmuch.
> The only applications I can see are "bfffo *,0,32,dN" which isn't
> exactly exhaustive.  Probably better to hand craft some tests vs real
> hardware.
> 
> Considering the prevelance of small helper functions calling ctz/clz,
> I'm thinking of adding an opcode to tcg for this.  Certainly all of
> the common hosts support it natively...

I've booted an etch-m68k container with patch and it works.

> @@ -3943,11 +3960,17 @@ DISAS_INSN(bfop_reg)
>              tcg_gen_rotl_i32(QREG_CC_N, src, tmp);
>              tcg_gen_andc_i32(QREG_CC_N, QREG_CC_N, mask);
>              tcg_gen_rotr_i32(mask, mask, tmp);
> +            if (!TCGV_IS_UNUSED(tofs)) {
> +                tcg_gen_mov_tl(tofs, tmp);

_tl suffix is never used with m68k, should we?


> @@ -5415,6 +5450,8 @@ void register_m68k_insns (CPUM68KState *env)
>      INSN(bfop_reg, eac0, fff8, BITFIELD);   /* bfchg */
>      INSN(bfop_mem, ecc0, ffc0, BITFIELD);   /* bfclr */
>      INSN(bfop_reg, ecc0, fff8, BITFIELD);   /* bfclr */
> +    INSN(bfop_mem, edc0, ffc0, BITFIELD);   /* bfffo */
> +    INSN(bfop_reg, edc0, fff8, BITFIELD);   /* bfffo */

In my doc, bfffo is 0xE9C0.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Laurent Vivier]

Le 15/11/2016 à 22:07, Laurent Vivier a écrit :
> Le 15/11/2016 à 21:44, Richard Henderson a écrit :
>> @@ -5415,6 +5450,8 @@ void register_m68k_insns (CPUM68KState *env)
>>      INSN(bfop_reg, eac0, fff8, BITFIELD);   /* bfchg */
>>      INSN(bfop_mem, ecc0, ffc0, BITFIELD);   /* bfclr */
>>      INSN(bfop_reg, ecc0, fff8, BITFIELD);   /* bfclr */
>> +    INSN(bfop_mem, edc0, ffc0, BITFIELD);   /* bfffo */
>> +    INSN(bfop_reg, edc0, fff8, BITFIELD);   /* bfffo */
> 
> In my doc, bfffo is 0xE9C0.

I think my doc [1] is wrong, because it is the same as bextu and the
as/objdump gives me 0xEDC0.

Laurent

[1] "MOTOROLA M68000 FAMILY Programmer’s Reference Manual (Includes
CPU32 Instructions)"

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Richard Henderson]

On 11/15/2016 10:12 PM, Laurent Vivier wrote:
> Le 15/11/2016 à 22:07, Laurent Vivier a écrit :
>> Le 15/11/2016 à 21:44, Richard Henderson a écrit :
>>> @@ -5415,6 +5450,8 @@ void register_m68k_insns (CPUM68KState *env)
>>>      INSN(bfop_reg, eac0, fff8, BITFIELD);   /* bfchg */
>>>      INSN(bfop_mem, ecc0, ffc0, BITFIELD);   /* bfclr */
>>>      INSN(bfop_reg, ecc0, fff8, BITFIELD);   /* bfclr */
>>> +    INSN(bfop_mem, edc0, ffc0, BITFIELD);   /* bfffo */
>>> +    INSN(bfop_reg, edc0, fff8, BITFIELD);   /* bfffo */
>>
>> In my doc, bfffo is 0xE9C0.
>
> I think my doc [1] is wrong, because it is the same as bextu and the
> as/objdump gives me 0xEDC0.

My doc gives the same wrong opcode on the bfffo description page.  It confused 
me for some time until I scrolled to the opcode map at the end of the document, 
where it is correctly identified as EDC0.


r~

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Richard Henderson]

On 11/15/2016 10:07 PM, Laurent Vivier wrote:
> Le 15/11/2016 à 21:44, Richard Henderson a écrit :
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>
>> I've started a glibc test run with this, but I don't expect overmuch.
>> The only applications I can see are "bfffo *,0,32,dN" which isn't
>> exactly exhaustive.  Probably better to hand craft some tests vs real
>> hardware.
>>
>> Considering the prevelance of small helper functions calling ctz/clz,
>> I'm thinking of adding an opcode to tcg for this.  Certainly all of
>> the common hosts support it natively...
>
> I've booted an etch-m68k container with patch and it works.

Thanks.

>> @@ -3943,11 +3960,17 @@ DISAS_INSN(bfop_reg)
>>              tcg_gen_rotl_i32(QREG_CC_N, src, tmp);
>>              tcg_gen_andc_i32(QREG_CC_N, QREG_CC_N, mask);
>>              tcg_gen_rotr_i32(mask, mask, tmp);
>> +            if (!TCGV_IS_UNUSED(tofs)) {
>> +                tcg_gen_mov_tl(tofs, tmp);
>
> _tl suffix is never used with m68k, should we?
>

Gah.  Typing habit from other targets.

I don't think there's any point in using _tl for m68k, as there will never be a 
64-bit version.  If you could fix that as you apply to your tree, that would be 
great.


r~

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Laurent Vivier]

Le 15/11/2016 à 22:21, Richard Henderson a écrit :
> On 11/15/2016 10:07 PM, Laurent Vivier wrote:
>> Le 15/11/2016 à 21:44, Richard Henderson a écrit :
>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>>> ---
>>>
>>> I've started a glibc test run with this, but I don't expect overmuch.
>>> The only applications I can see are "bfffo *,0,32,dN" which isn't
>>> exactly exhaustive.  Probably better to hand craft some tests vs real
>>> hardware.
>>>
>>> Considering the prevelance of small helper functions calling ctz/clz,
>>> I'm thinking of adding an opcode to tcg for this.  Certainly all of
>>> the common hosts support it natively...
>>
>> I've booted an etch-m68k container with patch and it works.
> 
> Thanks.
> 
>>> @@ -3943,11 +3960,17 @@ DISAS_INSN(bfop_reg)
>>>              tcg_gen_rotl_i32(QREG_CC_N, src, tmp);
>>>              tcg_gen_andc_i32(QREG_CC_N, QREG_CC_N, mask);
>>>              tcg_gen_rotr_i32(mask, mask, tmp);
>>> +            if (!TCGV_IS_UNUSED(tofs)) {
>>> +                tcg_gen_mov_tl(tofs, tmp);
>>
>> _tl suffix is never used with m68k, should we?
>>
> 
> Gah.  Typing habit from other targets.
> 
> I don't think there's any point in using _tl for m68k, as there will
> never be a 64-bit version.  If you could fix that as you apply to your
> tree, that would be great.

I'm going to add the three bitfield patches to the m68k-for-2.9 branch,
fixing this.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Andreas Schwab]

On Nov 15 2016, Richard Henderson <rth@twiddle.net> wrote:

> On 11/15/2016 10:12 PM, Laurent Vivier wrote:
>> Le 15/11/2016 à 22:07, Laurent Vivier a écrit :
>>> Le 15/11/2016 à 21:44, Richard Henderson a écrit :
>>>> @@ -5415,6 +5450,8 @@ void register_m68k_insns (CPUM68KState *env)
>>>>      INSN(bfop_reg, eac0, fff8, BITFIELD);   /* bfchg */
>>>>      INSN(bfop_mem, ecc0, ffc0, BITFIELD);   /* bfclr */
>>>>      INSN(bfop_reg, ecc0, fff8, BITFIELD);   /* bfclr */
>>>> +    INSN(bfop_mem, edc0, ffc0, BITFIELD);   /* bfffo */
>>>> +    INSN(bfop_reg, edc0, fff8, BITFIELD);   /* bfffo */
>>>
>>> In my doc, bfffo is 0xE9C0.
>>
>> I think my doc [1] is wrong, because it is the same as bextu and the
>> as/objdump gives me 0xEDC0.
>
> My doc gives the same wrong opcode on the bfffo description page.  It
> confused me for some time until I scrolled to the opcode map at the end of
> the document, where it is correctly identified as EDC0.

The printed manual has the correct opcode, it is only wrong in the PDF.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Laurent Vivier]

Le 15/11/2016 à 21:44, Richard Henderson a écrit :
> Signed-off-by: Richard Henderson <rth@twiddle.net>

> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index bc6ec39..ed40ade 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -3913,7 +3913,14 @@ DISAS_INSN(bfop_reg)
>      TCGv src = DREG(insn, 0);
>      int len = ((extract32(ext, 0, 5) - 1) & 31) + 1;
>      int ofs = extract32(ext, 6, 5);  /* big bit-endian */
> -    TCGv mask;
> +    TCGv mask, tofs, tlen;
> +
> +    TCGV_UNUSED(tofs);
> +    TCGV_UNUSED(tlen);
> +    if ((insn & 0x0f00) == 0x0d00) { /* bfffo */
> +        tofs = tcg_temp_new();
> +        tlen = tcg_temp_new();
> +    }
>  
>      if ((ext & 0x820) == 0) {
>          /* Immediate width and offset.  */
> @@ -3925,6 +3932,10 @@ DISAS_INSN(bfop_reg)
>          }
>          tcg_gen_andi_i32(QREG_CC_N, QREG_CC_N, ~maski);
>          mask = tcg_const_i32(ror32(maski, ofs));
> +        if (!TCGV_IS_UNUSED(tofs)) {
> +            tcg_gen_movi_i32(tofs, ofs);
> +            tcg_gen_movi_i32(tlen, len);
> +        }
>      } else {
>          TCGv tmp = tcg_temp_new();
>          if (ext & 0x20) {
> @@ -3933,9 +3944,15 @@ DISAS_INSN(bfop_reg)
>              tcg_gen_andi_i32(tmp, tmp, 31);
>              mask = tcg_const_i32(0x7fffffffu);
>              tcg_gen_shr_i32(mask, mask, tmp);
> +            if (!TCGV_IS_UNUSED(tlen)) {
> +                tcg_gen_mov_i32(tlen, tmp);

We must add 1 here, otherwise the value stays between 0 and 31, not 1
and 32:

+                tcg_gen_addi_i32(tlen, tmp, 1);

> +            }
>          } else {
>              /* Immediate width */
>              mask = tcg_const_i32(0x7fffffffu >> (len - 1));
> +            if (!TCGV_IS_UNUSED(tlen)) {
> +                tcg_gen_movi_i32(tlen, len);
> +            }
>          }

In the case of the immediate value, it is added at beginning of the
function:

>      int len = ((extract32(ext, 0, 5) - 1) & 31) + 1;

Laurent

Re: [Qemu-devel] [PATCH] target-m68k: Implement bfffo

[Richard Henderson]

On 11/27/2016 11:53 AM, Laurent Vivier wrote:
>>              tcg_gen_andi_i32(tmp, tmp, 31);
>> >              mask = tcg_const_i32(0x7fffffffu);
>> >              tcg_gen_shr_i32(mask, mask, tmp);
>> > +            if (!TCGV_IS_UNUSED(tlen)) {
>> > +                tcg_gen_mov_i32(tlen, tmp);
> We must add 1 here, otherwise the value stays between 0 and 31, not 1
> and 32:
> 
> +                tcg_gen_addi_i32(tlen, tmp, 1);
> 

Yep, good catch.


r~