Re: [Qemu-devel] [PATCH v4 14/64] target-arm: Use new deposit and extract ops

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <rth@twiddle.net> writes:

> Use the new primitives for UBFX and SBFX.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-arm/translate-a64.c | 79 +++++++++++++++-------------------------------
>  target-arm/translate.c     | 37 +++++-----------------
>  2 files changed, 34 insertions(+), 82 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index de48747..e90487b 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -3219,67 +3219,40 @@ static void disas_bitfield(DisasContext *s, uint32_t insn)
>         low 32-bits anyway.  */
>      tcg_tmp = read_cpu_reg(s, rn, 1);
>
> -    /* Recognize the common aliases.  */
> -    if (opc == 0) { /* SBFM */
> -        if (ri == 0) {
> -            if (si == 7) { /* SXTB */
> -                tcg_gen_ext8s_i64(tcg_rd, tcg_tmp);
> -                goto done;
> -            } else if (si == 15) { /* SXTH */
> -                tcg_gen_ext16s_i64(tcg_rd, tcg_tmp);
> -                goto done;
> -            } else if (si == 31) { /* SXTW */
> -                tcg_gen_ext32s_i64(tcg_rd, tcg_tmp);
> -                goto done;
> -            }
> -        }
> -        if (si == 63 || (si == 31 && ri <= si)) { /* ASR */
> -            if (si == 31) {
> -                tcg_gen_ext32s_i64(tcg_tmp, tcg_tmp);
> -            }
> -            tcg_gen_sari_i64(tcg_rd, tcg_tmp, ri);
> +    /* Recognize simple(r) extractions.  */
> +    if (ri <= si) {
> +        int len = (si - ri) + 1;

This is confusing as you have now aliased with len above.

> +        if (opc == 0) { /* SBFM: ASR, SBFX, SXTB, SXTH, SXTW */
> +            tcg_gen_sextract_i64(tcg_rd, tcg_tmp, ri, len);
>              goto done;
> -        }
> -    } else if (opc == 2) { /* UBFM */
> -        if (ri == 0) { /* UXTB, UXTH, plus non-canonical AND */
> -            tcg_gen_andi_i64(tcg_rd, tcg_tmp, bitmask64(si + 1));
> -            return;
> -        }
> -        if (si == 63 || (si == 31 && ri <= si)) { /* LSR */
> -            if (si == 31) {
> -                tcg_gen_ext32u_i64(tcg_tmp, tcg_tmp);
> -            }
> -            tcg_gen_shri_i64(tcg_rd, tcg_tmp, ri);
> +        } else if (opc == 2) { /* UBFM: UBFX, LSR, UXTB, UXTH */
> +            tcg_gen_extract_i64(tcg_rd, tcg_tmp, ri, len);
>              return;
>          }
> -        if (si + 1 == ri && si != bitsize - 1) { /* LSL */
> -            int shift = bitsize - 1 - si;
> -            tcg_gen_shli_i64(tcg_rd, tcg_tmp, shift);
> -            goto done;
> -        }
>      }
>
> -    if (opc != 1) { /* SBFM or UBFM */
> -        tcg_gen_movi_i64(tcg_rd, 0);
> -    }
> +    /* Do the bit move operation.  Note that above we handled ri <= si,
> +       Wd<s-r:0> = Wn<s:r>, via tcg_gen_*extract_i64.  Now we handle
> +       the ri > si case, Wd<32+s-r,32-r> = Wn<s:0>, via deposit.  */
> +    pos = (bitsize - ri) & (bitsize - 1);
> +    len = si + 1;

The comment implies this is for the ri > si case but you'll still catch
ri <= si for opc = 1, e.g.:

  0x331168a7      bfxil w7, w5, #17, #10

>
> -    /* do the bit move operation */
> -    if (si >= ri) {

In fact we seem to have subtly reversed the test here but ri <= si is
not exactly equivalent to si >= ri.

My version is as follows:

    /* Recognize simple(r) extractions.  */
    if (si >= ri) {
        /* Wd<s-r:0> = Wn<s:r> */
        len = (si - ri) + 1;
        if (opc == 0) { /* SBFM: ASR, SBFX, SXTB, SXTH, SXTW */
            tcg_gen_sextract_i64(tcg_rd, tcg_tmp, ri, len);
            goto done;
        } else if (opc == 2) { /* UBFM: UBFX, LSR, UXTB, UXTH */
            tcg_gen_extract_i64(tcg_rd, tcg_tmp, ri, len);
            return;
        }
        /* opc == 1, BXFIL fall through to deposit */
        tcg_gen_extract_i64(tcg_tmp, tcg_tmp, ri, len);
        pos = 0;
    } else {
        /* Handle the ri > si case with a deposit
         * Wd<32+s-r,32-r> = Wn<s:0>
         */
        len = si + 1;
        pos = (bitsize - ri) & (bitsize - 1);
    }

I've tested that with risu and all the bitfield tests seem ok. The full
patch on top of your commit was:

target-arm: fix bxfil case

1 file changed, 13 insertions(+), 9 deletions(-)
target-arm/translate-a64.c | 22 +++++++++++++---------

modified   target-arm/translate-a64.c
@@ -3220,8 +3220,9 @@ static void disas_bitfield(DisasContext *s, uint32_t insn)
     tcg_tmp = read_cpu_reg(s, rn, 1);

     /* Recognize simple(r) extractions.  */
-    if (ri <= si) {
-        int len = (si - ri) + 1;
+    if (si >= ri) {
+        /* Wd<s-r:0> = Wn<s:r> */
+        len = (si - ri) + 1;
         if (opc == 0) { /* SBFM: ASR, SBFX, SXTB, SXTH, SXTW */
             tcg_gen_sextract_i64(tcg_rd, tcg_tmp, ri, len);
             goto done;
@@ -3229,14 +3230,17 @@ static void disas_bitfield(DisasContext *s, uint32_t insn)
             tcg_gen_extract_i64(tcg_rd, tcg_tmp, ri, len);
             return;
         }
+        /* opc == 1, BXFIL fall through to deposit */
+        tcg_gen_extract_i64(tcg_tmp, tcg_tmp, ri, len);
+        pos = 0;
+    } else {
+        /* Handle the ri > si case with a deposit
+         * Wd<32+s-r,32-r> = Wn<s:0>
+         */
+        len = si + 1;
+        pos = (bitsize - ri) & (bitsize - 1);
     }

-    /* Do the bit move operation.  Note that above we handled ri <= si,
-       Wd<s-r:0> = Wn<s:r>, via tcg_gen_*extract_i64.  Now we handle
-       the ri > si case, Wd<32+s-r,32-r> = Wn<s:0>, via deposit.  */
-    pos = (bitsize - ri) & (bitsize - 1);
-    len = si + 1;
-
     if (opc == 0 && len < ri) {
         /* SBFM: sign extend the destination field from len to fill
            the balance of the word.  Let the deposit below insert all
@@ -3245,7 +3249,7 @@ static void disas_bitfield(DisasContext *s, uint32_t insn)
         len = ri;
     }

-    if (opc == 1) { /* BFM */
+    if (opc == 1) { /* BFM, BXFIL */
         tcg_gen_deposit_i64(tcg_rd, tcg_rd, tcg_tmp, pos, len);
     } else {
         /* SBFM or UBFM: We start with zero, and we haven't modified
--
Alex Bennée