[PATCH 0/7] tcg: Document *swap/deposit helpers

[PATCH 0/7] tcg: Document *swap/deposit helpers

[Philippe Mathieu-Daudé]

While reviewing a recent patch from Richard optimizing
deposit() [*] I ended looking at the *swap friends, taking
some notes, which then evolved to proper documentation.

[*] https://lore.kernel.org/qemu-devel/20230816145547.477974-3-richard.henderson@linaro.org/

Philippe Mathieu-Daudé (7):
  tcg/tcg-op: Document bswap16() byte pattern
  tcg/tcg-op: Document bswap32() byte pattern
  tcg/tcg-op: Document bswap64() byte pattern
  tcg/tcg-op: Document hswap() byte pattern
  tcg/tcg-op: Document wswap() byte pattern
  tcg/tcg-op: Document deposit_z()
  target/cris: Fix a typo in gen_swapr()

 docs/devel/tcg-ops.rst  | 12 ++++++
 target/cris/translate.c | 20 +++++----
 tcg/tcg-op.c            | 96 +++++++++++++++++++++++++++++++----------
 3 files changed, 96 insertions(+), 32 deletions(-)

-- 
2.41.0

[PATCH 1/7] tcg/tcg-op: Document bswap16() byte pattern

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tcg/tcg-op.c | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 7aadb37756..f164ddc95e 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -1021,6 +1021,13 @@ void tcg_gen_ext16u_i32(TCGv_i32 ret, TCGv_i32 arg)
     }
 }
 
+/*
+ * bswap16_i32: 16-bit byte swap on the low bits of a 32-bit value.
+ *
+ * Byte pattern:  bswap16_i32(xxab) -> ..ba             (TCG_BSWAP_OZ)
+ *                bswap16_i32(xxab) -> ssba             (TCG_BSWAP_OS)
+ *                bswap16_i32(xxab) -> xxba
+ */
 void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg, int flags)
 {
     /* Only one extension flag may be present. */
@@ -1032,22 +1039,23 @@ void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg, int flags)
         TCGv_i32 t0 = tcg_temp_ebb_new_i32();
         TCGv_i32 t1 = tcg_temp_ebb_new_i32();
 
-        tcg_gen_shri_i32(t0, arg, 8);
+                                            /* arg = xxab */
+        tcg_gen_shri_i32(t0, arg, 8);       /*  t0 = .xxa */
         if (!(flags & TCG_BSWAP_IZ)) {
-            tcg_gen_ext8u_i32(t0, t0);
+            tcg_gen_ext8u_i32(t0, t0);      /*  t0 = ...a */
         }
 
         if (flags & TCG_BSWAP_OS) {
-            tcg_gen_shli_i32(t1, arg, 24);
-            tcg_gen_sari_i32(t1, t1, 16);
+            tcg_gen_shli_i32(t1, arg, 24);  /*  t1 = b... */
+            tcg_gen_sari_i32(t1, t1, 16);   /*  t1 = ssb. */
         } else if (flags & TCG_BSWAP_OZ) {
-            tcg_gen_ext8u_i32(t1, arg);
-            tcg_gen_shli_i32(t1, t1, 8);
+            tcg_gen_ext8u_i32(t1, arg);     /*  t1 = ...b */
+            tcg_gen_shli_i32(t1, t1, 8);    /*  t1 = ..b. */
         } else {
-            tcg_gen_shli_i32(t1, arg, 8);
+            tcg_gen_shli_i32(t1, arg, 8);   /*  t1 = xab. */
         }
 
-        tcg_gen_or_i32(ret, t0, t1);
+        tcg_gen_or_i32(ret, t0, t1);        /* ret = ssba */
         tcg_temp_free_i32(t0);
         tcg_temp_free_i32(t1);
     }
@@ -1721,6 +1729,13 @@ void tcg_gen_ext32u_i64(TCGv_i64 ret, TCGv_i64 arg)
     }
 }
 
+/*
+ * bswap16_i64: 16-bit byte swap on the low bits of a 64-bit value.
+ *
+ * Byte pattern:  bswap16_i32(xxxxxxab) -> ......ba     (TCG_BSWAP_OZ)
+ *                bswap16_i32(xxxxxxab) -> ssssssba     (TCG_BSWAP_OS)
+ *                bswap16_i32(xxxxxxab) -> xxxxxxba
+ */
 void tcg_gen_bswap16_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
 {
     /* Only one extension flag may be present. */
@@ -1739,22 +1754,23 @@ void tcg_gen_bswap16_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
         TCGv_i64 t0 = tcg_temp_ebb_new_i64();
         TCGv_i64 t1 = tcg_temp_ebb_new_i64();
 
-        tcg_gen_shri_i64(t0, arg, 8);
+                                            /* arg = xxxxxxab */
+        tcg_gen_shri_i64(t0, arg, 8);       /*  t0 = .xxxxxxa */
         if (!(flags & TCG_BSWAP_IZ)) {
-            tcg_gen_ext8u_i64(t0, t0);
+            tcg_gen_ext8u_i64(t0, t0);      /*  t0 = .......a */
         }
 
         if (flags & TCG_BSWAP_OS) {
-            tcg_gen_shli_i64(t1, arg, 56);
-            tcg_gen_sari_i64(t1, t1, 48);
+            tcg_gen_shli_i64(t1, arg, 56);  /*  t1 = b....... */
+            tcg_gen_sari_i64(t1, t1, 48);   /*  t1 = ssssssb. */
         } else if (flags & TCG_BSWAP_OZ) {
-            tcg_gen_ext8u_i64(t1, arg);
-            tcg_gen_shli_i64(t1, t1, 8);
+            tcg_gen_ext8u_i64(t1, arg);     /*  t1 = .......b */
+            tcg_gen_shli_i64(t1, t1, 8);    /*  t1 = ......b. */
         } else {
-            tcg_gen_shli_i64(t1, arg, 8);
+            tcg_gen_shli_i64(t1, arg, 8);   /*  t1 = xxxxxxb. */
         }
 
-        tcg_gen_or_i64(ret, t0, t1);
+        tcg_gen_or_i64(ret, t0, t1);        /* ret = ssssssba */
         tcg_temp_free_i64(t0);
         tcg_temp_free_i64(t1);
     }
-- 
2.41.0

Re: [PATCH 1/7] tcg/tcg-op: Document bswap16() byte pattern

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tcg/tcg-op.c | 48 ++++++++++++++++++++++++++++++++----------------
>   1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 7aadb37756..f164ddc95e 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -1021,6 +1021,13 @@ void tcg_gen_ext16u_i32(TCGv_i32 ret, TCGv_i32 arg)
>       }
>   }
>   
> +/*
> + * bswap16_i32: 16-bit byte swap on the low bits of a 32-bit value.
> + *
> + * Byte pattern:  bswap16_i32(xxab) -> ..ba             (TCG_BSWAP_OZ)
> + *                bswap16_i32(xxab) -> ssba             (TCG_BSWAP_OS)
> + *                bswap16_i32(xxab) -> xxba
> + */

Don't forget TCG_BSWAP_IZ, which means the input is already zero-extended.
Which makes

> +                                            /* arg = xxab */
> +        tcg_gen_shri_i32(t0, arg, 8);       /*  t0 = .xxa */

this

>           if (!(flags & TCG_BSWAP_IZ)) {
> -            tcg_gen_ext8u_i32(t0, t0);
> +            tcg_gen_ext8u_i32(t0, t0);      /*  t0 = ...a */
>           }
>   
>           if (flags & TCG_BSWAP_OS) {
> -            tcg_gen_shli_i32(t1, arg, 24);
> -            tcg_gen_sari_i32(t1, t1, 16);
> +            tcg_gen_shli_i32(t1, arg, 24);  /*  t1 = b... */
> +            tcg_gen_sari_i32(t1, t1, 16);   /*  t1 = ssb. */
>           } else if (flags & TCG_BSWAP_OZ) {
> -            tcg_gen_ext8u_i32(t1, arg);
> -            tcg_gen_shli_i32(t1, t1, 8);
> +            tcg_gen_ext8u_i32(t1, arg);     /*  t1 = ...b */
> +            tcg_gen_shli_i32(t1, t1, 8);    /*  t1 = ..b. */
>           } else {
> -            tcg_gen_shli_i32(t1, arg, 8);
> +            tcg_gen_shli_i32(t1, arg, 8);   /*  t1 = xab. */

and this slightly inaccurate.

>           }
>   
> -        tcg_gen_or_i32(ret, t0, t1);
> +        tcg_gen_or_i32(ret, t0, t1);        /* ret = ssba */

This one is just confusing, since each of the three cases above have different outputs.


r~




r~

Re: [PATCH 1/7] tcg/tcg-op: Document bswap16() byte pattern

[Philippe Mathieu-Daudé]

On 22/8/23 17:58, Richard Henderson wrote:
> On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tcg/tcg-op.c | 48 ++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
>> index 7aadb37756..f164ddc95e 100644
>> --- a/tcg/tcg-op.c
>> +++ b/tcg/tcg-op.c
>> @@ -1021,6 +1021,13 @@ void tcg_gen_ext16u_i32(TCGv_i32 ret, TCGv_i32 
>> arg)
>>       }
>>   }
>> +/*
>> + * bswap16_i32: 16-bit byte swap on the low bits of a 32-bit value.
>> + *
>> + * Byte pattern:  bswap16_i32(xxab) -> ..ba             (TCG_BSWAP_OZ)
>> + *                bswap16_i32(xxab) -> ssba             (TCG_BSWAP_OS)
>> + *                bswap16_i32(xxab) -> xxba
>> + */
> 
> Don't forget TCG_BSWAP_IZ, which means the input is already zero-extended.
> Which makes
> 
>> +                                            /* arg = xxab */
>> +        tcg_gen_shri_i32(t0, arg, 8);       /*  t0 = .xxa */
> 
> this
> 
>>           if (!(flags & TCG_BSWAP_IZ)) {
>> -            tcg_gen_ext8u_i32(t0, t0);
>> +            tcg_gen_ext8u_i32(t0, t0);      /*  t0 = ...a */
>>           }
>>           if (flags & TCG_BSWAP_OS) {
>> -            tcg_gen_shli_i32(t1, arg, 24);
>> -            tcg_gen_sari_i32(t1, t1, 16);
>> +            tcg_gen_shli_i32(t1, arg, 24);  /*  t1 = b... */
>> +            tcg_gen_sari_i32(t1, t1, 16);   /*  t1 = ssb. */
>>           } else if (flags & TCG_BSWAP_OZ) {
>> -            tcg_gen_ext8u_i32(t1, arg);
>> -            tcg_gen_shli_i32(t1, t1, 8);
>> +            tcg_gen_ext8u_i32(t1, arg);     /*  t1 = ...b */
>> +            tcg_gen_shli_i32(t1, t1, 8);    /*  t1 = ..b. */
>>           } else {
>> -            tcg_gen_shli_i32(t1, arg, 8);
>> +            tcg_gen_shli_i32(t1, arg, 8);   /*  t1 = xab. */
> 
> and this slightly inaccurate.
> 
>>           }
>> -        tcg_gen_or_i32(ret, t0, t1);
>> +        tcg_gen_or_i32(ret, t0, t1);        /* ret = ssba */
> 
> This one is just confusing, since each of the three cases above have 
> different outputs.

Is that formatting OK with you?

/*
  * bswap16_i32: 16-bit byte swap on the low bits of a 32-bit value.
  *
  * Byte pattern:  bswap16_i32(..ab) -> ..ba             (TCG_BSWAP_IZ)
  *                bswap16_i32(xxab) -> ..ba             (TCG_BSWAP_OZ)
  *                bswap16_i32(xxab) -> ssba             (TCG_BSWAP_OS)
  *                bswap16_i32(xxab) -> xxba
  */
void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg, int flags)
{
     /* Only one extension flag may be present. */
     tcg_debug_assert(!(flags & TCG_BSWAP_OS) || !(flags & TCG_BSWAP_OZ));

     if (TCG_TARGET_HAS_bswap16_i32) {
         tcg_gen_op3i_i32(INDEX_op_bswap16_i32, ret, arg, flags);
     } else {
         TCGv_i32 t0 = tcg_temp_ebb_new_i32();
         TCGv_i32 t1 = tcg_temp_ebb_new_i32();

                                             /* arg = xxab (IZ=0) */
                                             /*       ..ab (IZ=1) */
         tcg_gen_shri_i32(t0, arg, 8);       /*  t0 = .xxa (IZ=0) */
                                             /*       ...a (IZ=1) */
         if (!(flags & TCG_BSWAP_IZ)) {
             tcg_gen_ext8u_i32(t0, t0);      /*  t0 = ...a */
         }

         if (flags & TCG_BSWAP_OS) {
             tcg_gen_shli_i32(t1, arg, 24);  /*  t1 = b... */
             tcg_gen_sari_i32(t1, t1, 16);   /*  t1 = ssb. */
         } else if (flags & TCG_BSWAP_OZ) {
             tcg_gen_ext8u_i32(t1, arg);     /*  t1 = ...b */
             tcg_gen_shli_i32(t1, t1, 8);    /*  t1 = ..b. */
         } else {
             tcg_gen_shli_i32(t1, arg, 8);   /*  t1 = xab. (IZ=0) */
                                             /*       .ab. (IZ=1) */
         }

         tcg_gen_or_i32(ret, t0, t1);        /* ret = ..ba (IZ=1 or OZ=1) */
                                             /*     = ssba (OS=1)         */
                                             /*     = xxba (no flag)      */
         tcg_temp_free_i32(t0);
         tcg_temp_free_i32(t1);
     }
}

---

Re: [PATCH 1/7] tcg/tcg-op: Document bswap16() byte pattern

[Richard Henderson]

On 8/22/23 10:22, Philippe Mathieu-Daudé wrote:
>          } else {
>              tcg_gen_shli_i32(t1, arg, 8);   /*  t1 = xab. (IZ=0) */
>                                              /*       .ab. (IZ=1) */
>          }
> 
>          tcg_gen_or_i32(ret, t0, t1);        /* ret = ..ba (IZ=1 or OZ=1) */
>                                              /*     = ssba (OS=1)         */
>                                              /*     = xxba (no flag)      */

Clearer with xaba for no flag?

Otherwise it looks ok.


r~

Re: [PATCH 1/7] tcg/tcg-op: Document bswap16() byte pattern

[Philippe Mathieu-Daudé]

On 22/8/23 19:29, Richard Henderson wrote:
> On 8/22/23 10:22, Philippe Mathieu-Daudé wrote:
>>          } else {
>>              tcg_gen_shli_i32(t1, arg, 8);   /*  t1 = xab. (IZ=0) */
>>                                              /*       .ab. (IZ=1) */
>>          }
>>
>>          tcg_gen_or_i32(ret, t0, t1);        /* ret = ..ba (IZ=1 or 
>> OZ=1) */
>>                                              /*     = ssba 
>> (OS=1)         */
>>                                              /*     = xxba (no 
>> flag)      */
> 
> Clearer with xaba for no flag?

Right :)

> Otherwise it looks ok.

Thanks!

[PATCH 2/7] tcg/tcg-op: Document bswap32() byte pattern

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tcg/tcg-op.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index f164ddc95e..e6b0d74a46 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -1061,6 +1061,11 @@ void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg, int flags)
     }
 }
 
+/*
+ * bswap32_i32: 32-bit byte swap on a 32-bit value.
+ *
+ * Byte pattern:  bswap32_i32(abcd) -> dcba
+ */
 void tcg_gen_bswap32_i32(TCGv_i32 ret, TCGv_i32 arg)
 {
     if (TCG_TARGET_HAS_bswap32_i32) {
@@ -1776,6 +1781,13 @@ void tcg_gen_bswap16_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
     }
 }
 
+/*
+ * bswap32_i64: 32-bit byte swap on the low bits of a 64-bit value.
+ *
+ * Byte pattern:  bswap32_i64(xxxxabcd) -> ....dcba     (TCG_BSWAP_OZ)
+ *                bswap32_i64(xxxxabcd) -> ssssdcba     (TCG_BSWAP_OS)
+ *                bswap32_i64(xxxxabcd) -> xxxxdcba
+ */
 void tcg_gen_bswap32_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
 {
     /* Only one extension flag may be present. */
-- 
2.41.0

Re: [PATCH 2/7] tcg/tcg-op: Document bswap32() byte pattern

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tcg/tcg-op.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index f164ddc95e..e6b0d74a46 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -1061,6 +1061,11 @@ void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg, int flags)
>       }
>   }
>   
> +/*
> + * bswap32_i32: 32-bit byte swap on a 32-bit value.
> + *
> + * Byte pattern:  bswap32_i32(abcd) -> dcba
> + */

Ok.

>   void tcg_gen_bswap32_i32(TCGv_i32 ret, TCGv_i32 arg)
>   {
>       if (TCG_TARGET_HAS_bswap32_i32) {
> @@ -1776,6 +1781,13 @@ void tcg_gen_bswap16_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
>       }
>   }
>   
> +/*
> + * bswap32_i64: 32-bit byte swap on the low bits of a 64-bit value.
> + *
> + * Byte pattern:  bswap32_i64(xxxxabcd) -> ....dcba     (TCG_BSWAP_OZ)
> + *                bswap32_i64(xxxxabcd) -> ssssdcba     (TCG_BSWAP_OS)
> + *                bswap32_i64(xxxxabcd) -> xxxxdcba
> + */

Again, TCG_BSWAP_IZ.

r~

Re: [PATCH 2/7] tcg/tcg-op: Document bswap32() byte pattern

[Philippe Mathieu-Daudé]

On 22/8/23 18:00, Richard Henderson wrote:
> On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tcg/tcg-op.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)


>> +/*
>> + * bswap32_i64: 32-bit byte swap on the low bits of a 64-bit value.
>> + *
>> + * Byte pattern:  bswap32_i64(xxxxabcd) -> ....dcba     (TCG_BSWAP_OZ)
>> + *                bswap32_i64(xxxxabcd) -> ssssdcba     (TCG_BSWAP_OS)
>> + *                bswap32_i64(xxxxabcd) -> xxxxdcba
>> + */
> 
> Again, TCG_BSWAP_IZ.

Whether TCG_BSWAP_IZ is set or not doesn't change the result, at the
end we have:

   bswap32_i64(xxxxabcd) -> ....dcba     (TCG_BSWAP_OS not set)
   bswap32_i64(xxxxabcd) -> ssssdcba     (TCG_BSWAP_OS set)

Re: [PATCH 2/7] tcg/tcg-op: Document bswap32() byte pattern

[Richard Henderson]

On 8/23/23 06:14, Philippe Mathieu-Daudé wrote:
> On 22/8/23 18:00, Richard Henderson wrote:
>> On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   tcg/tcg-op.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
> 
> 
>>> +/*
>>> + * bswap32_i64: 32-bit byte swap on the low bits of a 64-bit value.
>>> + *
>>> + * Byte pattern:  bswap32_i64(xxxxabcd) -> ....dcba     (TCG_BSWAP_OZ)
>>> + *                bswap32_i64(xxxxabcd) -> ssssdcba     (TCG_BSWAP_OS)
>>> + *                bswap32_i64(xxxxabcd) -> xxxxdcba
>>> + */
>>
>> Again, TCG_BSWAP_IZ.
> 
> Whether TCG_BSWAP_IZ is set or not doesn't change the result, at the
> end we have:
> 
>    bswap32_i64(xxxxabcd) -> ....dcba     (TCG_BSWAP_OS not set)
>    bswap32_i64(xxxxabcd) -> ssssdcba     (TCG_BSWAP_OS set)

It changes the input: not 'x' but '.'.


r~

[PATCH 3/7] tcg/tcg-op: Document bswap64() byte pattern

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tcg/tcg-op.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index e6b0d74a46..f4fe13e040 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -1828,6 +1828,11 @@ void tcg_gen_bswap32_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
     }
 }
 
+/*
+ * bswap64_i64: 64-bit byte swap on a 64-bit value.
+ *
+ * Byte pattern:  bswap64_i64(abcdefgh) -> hgfedcba
+ */
 void tcg_gen_bswap64_i64(TCGv_i64 ret, TCGv_i64 arg)
 {
     if (TCG_TARGET_REG_BITS == 32) {
-- 
2.41.0

Re: [PATCH 3/7] tcg/tcg-op: Document bswap64() byte pattern

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tcg/tcg-op.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index e6b0d74a46..f4fe13e040 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -1828,6 +1828,11 @@ void tcg_gen_bswap32_i64(TCGv_i64 ret, TCGv_i64 arg, int flags)
>       }
>   }
>   
> +/*
> + * bswap64_i64: 64-bit byte swap on a 64-bit value.
> + *
> + * Byte pattern:  bswap64_i64(abcdefgh) -> hgfedcba
> + */
>   void tcg_gen_bswap64_i64(TCGv_i64 ret, TCGv_i64 arg)
>   {
>       if (TCG_TARGET_REG_BITS == 32) {

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/7] tcg/tcg-op: Document hswap() byte pattern

[Philippe Mathieu-Daudé]

Document hswap_i32() and hswap_i64(), added in commit
46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/tcg-ops.rst |  4 ++++
 tcg/tcg-op.c           | 26 +++++++++++++++++++-------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/docs/devel/tcg-ops.rst b/docs/devel/tcg-ops.rst
index 6a166c5665..d9364effd2 100644
--- a/docs/devel/tcg-ops.rst
+++ b/docs/devel/tcg-ops.rst
@@ -486,6 +486,10 @@ Misc
          into 32-bit output *t0*.  Depending on the host, this may be a simple shift,
          or may require additional canonicalization.
 
+   * - hswap_i32/i64 *t0*, *t1*
+
+     - | Swap 16-bit halfwords within a 32/64-bit value.
+
 
 Conditional moves
 -----------------
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index f4fe13e040..bb64326a1a 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -1091,6 +1091,11 @@ void tcg_gen_bswap32_i32(TCGv_i32 ret, TCGv_i32 arg)
     }
 }
 
+/*
+ * hswap_i32: Swap 16-bit halfwords within a 32-bit value.
+ *
+ * Byte pattern:  hswap_i32(abcd) -> cdab
+ */
 void tcg_gen_hswap_i32(TCGv_i32 ret, TCGv_i32 arg)
 {
     /* Swapping 2 16-bit elements is a rotate. */
@@ -1878,19 +1883,26 @@ void tcg_gen_bswap64_i64(TCGv_i64 ret, TCGv_i64 arg)
     }
 }
 
+/*
+ * hswap_i64: Swap 16-bit halfwords within a 64-bit value.
+ *
+ * See hswap64() in include/qemu/bitops.h
+ *
+ * Byte pattern:  hswap_i64(abcdefgh) -> ghefcdab
+ */
 void tcg_gen_hswap_i64(TCGv_i64 ret, TCGv_i64 arg)
 {
     uint64_t m = 0x0000ffff0000ffffull;
     TCGv_i64 t0 = tcg_temp_ebb_new_i64();
     TCGv_i64 t1 = tcg_temp_ebb_new_i64();
 
-    /* See include/qemu/bitops.h, hswap64. */
-    tcg_gen_rotli_i64(t1, arg, 32);
-    tcg_gen_andi_i64(t0, t1, m);
-    tcg_gen_shli_i64(t0, t0, 16);
-    tcg_gen_shri_i64(t1, t1, 16);
-    tcg_gen_andi_i64(t1, t1, m);
-    tcg_gen_or_i64(ret, t0, t1);
+                                        /* arg = abcdefgh */
+    tcg_gen_rotli_i64(t1, arg, 32);     /*  t1 = efghabcd */
+    tcg_gen_andi_i64(t0, t1, m);        /*  t0 = ..gh..cd */
+    tcg_gen_shli_i64(t0, t0, 16);       /*  t0 = gh..cd.. */
+    tcg_gen_shri_i64(t1, t1, 16);       /*  t1 = ..efghab */
+    tcg_gen_andi_i64(t1, t1, m);        /*  t1 = ..ef..ab */
+    tcg_gen_or_i64(ret, t0, t1);        /* ret = ghefcdab */
 
     tcg_temp_free_i64(t0);
     tcg_temp_free_i64(t1);
-- 
2.41.0

Re: [PATCH 4/7] tcg/tcg-op: Document hswap() byte pattern

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Document hswap_i32() and hswap_i64(), added in commit
> 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}").
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   docs/devel/tcg-ops.rst |  4 ++++
>   tcg/tcg-op.c           | 26 +++++++++++++++++++-------
>   2 files changed, 23 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/7] tcg/tcg-op: Document hswap() byte pattern

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Document hswap_i32() and hswap_i64(), added in commit
> 46be8425ff ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   docs/devel/tcg-ops.rst |  4 ++++
>   tcg/tcg-op.c           | 26 +++++++++++++++++++-------
>   2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/devel/tcg-ops.rst b/docs/devel/tcg-ops.rst
> index 6a166c5665..d9364effd2 100644
> --- a/docs/devel/tcg-ops.rst
> +++ b/docs/devel/tcg-ops.rst
> @@ -486,6 +486,10 @@ Misc
>            into 32-bit output *t0*.  Depending on the host, this may be a simple shift,
>            or may require additional canonicalization.
>   
> +   * - hswap_i32/i64 *t0*, *t1*
> +
> +     - | Swap 16-bit halfwords within a 32/64-bit value.

hswap is not a tcg opcode, so this is incorrect.

This falls into the part of TCG that Peter has mentioned many times: we have opcode 
documentation, but no separate translator front end documentation.


r~

[PATCH 5/7] tcg/tcg-op: Document wswap() byte pattern

[Philippe Mathieu-Daudé]

Document wswap_i64(), added in commit 46be8425ff
("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/tcg-ops.rst | 4 ++++
 tcg/tcg-op.c           | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/docs/devel/tcg-ops.rst b/docs/devel/tcg-ops.rst
index d9364effd2..306025ece7 100644
--- a/docs/devel/tcg-ops.rst
+++ b/docs/devel/tcg-ops.rst
@@ -490,6 +490,10 @@ Misc
 
      - | Swap 16-bit halfwords within a 32/64-bit value.
 
+   * - wswap_i64 *t0*, *t1*
+
+     - | Swap 32-bit words within a 64-bit value.
+
 
 Conditional moves
 -----------------
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index bb64326a1a..c436c5e263 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -1908,6 +1908,11 @@ void tcg_gen_hswap_i64(TCGv_i64 ret, TCGv_i64 arg)
     tcg_temp_free_i64(t1);
 }
 
+/*
+ * wswap_i64: Swap 32-bit words within a 64-bit value.
+ *
+ * Byte pattern:  wswap_i64(abcdefgh) -> efghabcd
+ */
 void tcg_gen_wswap_i64(TCGv_i64 ret, TCGv_i64 arg)
 {
     /* Swapping 2 32-bit elements is a rotate. */
-- 
2.41.0

Re: [PATCH 5/7] tcg/tcg-op: Document wswap() byte pattern

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Document wswap_i64(), added in commit 46be8425ff
> ("tcg: Implement tcg_gen_{h,w}swap_{i32,i64}").
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   docs/devel/tcg-ops.rst | 4 ++++
>   tcg/tcg-op.c           | 5 +++++
>   2 files changed, 9 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 6/7] tcg/tcg-op: Document deposit_z()

[Philippe Mathieu-Daudé]

Document deposit_z_i32() and deposit_z_i64(), added in
commit 07cc68d528 ("tcg: Add deposit_z expander").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/tcg-ops.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/devel/tcg-ops.rst b/docs/devel/tcg-ops.rst
index 306025ece7..7ea6aba502 100644
--- a/docs/devel/tcg-ops.rst
+++ b/docs/devel/tcg-ops.rst
@@ -449,6 +449,10 @@ Misc
        |
        |     *dest* = (*t1* & ~0x0f00) | ((*t2* << 8) & 0x0f00)
 
+   * - deposit_z_i32/i64 *dest*, *t1*, *pos*, *len*
+
+     - | Similar to deposit, except that a zero value is deposited.
+
    * - extract_i32/i64 *dest*, *t1*, *pos*, *len*
 
        sextract_i32/i64 *dest*, *t1*, *pos*, *len*
-- 
2.41.0

Re: [PATCH 6/7] tcg/tcg-op: Document deposit_z()

[Richard Henderson]

On 8/22/23 02:37, Philippe Mathieu-Daudé wrote:
> Document deposit_z_i32() and deposit_z_i64(), added in
> commit 07cc68d528 ("tcg: Add deposit_z expander").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   docs/devel/tcg-ops.rst | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/docs/devel/tcg-ops.rst b/docs/devel/tcg-ops.rst
> index 306025ece7..7ea6aba502 100644
> --- a/docs/devel/tcg-ops.rst
> +++ b/docs/devel/tcg-ops.rst
> @@ -449,6 +449,10 @@ Misc
>          |
>          |     *dest* = (*t1* & ~0x0f00) | ((*t2* << 8) & 0x0f00)
>   
> +   * - deposit_z_i32/i64 *dest*, *t1*, *pos*, *len*
> +
> +     - | Similar to deposit, except that a zero value is deposited.

Nack -- deposit_z is not a tcg opcode.


r~

[PATCH 7/7] target/cris: Fix a typo in gen_swapr()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/cris/translate.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target/cris/translate.c b/target/cris/translate.c
index 0b3d724281..42103b5558 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -411,15 +411,17 @@ static inline void t_gen_swapw(TCGv d, TCGv s)
     tcg_gen_or_tl(d, d, t);
 }
 
-/* Reverse the within each byte.
-   T0 = (((T0 << 7) & 0x80808080) |
-   ((T0 << 5) & 0x40404040) |
-   ((T0 << 3) & 0x20202020) |
-   ((T0 << 1) & 0x10101010) |
-   ((T0 >> 1) & 0x08080808) |
-   ((T0 >> 3) & 0x04040404) |
-   ((T0 >> 5) & 0x02020202) |
-   ((T0 >> 7) & 0x01010101));
+/*
+ * Reverse the bits within each byte.
+ *
+ *  T0 = ((T0 << 7) & 0x80808080)
+ *     | ((T0 << 5) & 0x40404040)
+ *     | ((T0 << 3) & 0x20202020)
+ *     | ((T0 << 1) & 0x10101010)
+ *     | ((T0 >> 1) & 0x08080808)
+ *     | ((T0 >> 3) & 0x04040404)
+ *     | ((T0 >> 5) & 0x02020202)
+ *     | ((T0 >> 7) & 0x01010101);
  */
 static void t_gen_swapr(TCGv d, TCGv s)
 {
-- 
2.41.0

Re: [PATCH 0/7] tcg: Document *swap/deposit helpers

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> While reviewing a recent patch from Richard optimizing
> deposit() [*] I ended looking at the *swap friends, taking
> some notes, which then evolved to proper documentation.
>
> [*]
> https://lore.kernel.org/qemu-devel/20230816145547.477974-3-richard.henderson@linaro.org/

We already have some documentation in tcg.rst:

   * - bswap16_i32/i64 *t0*, *t1*, *flags*

     - | 16 bit byte swap on the low bits of a 32/64 bit input.
       |
       | If *flags* & ``TCG_BSWAP_IZ``, then *t1* is known to be zero-extended from bit 15.
       | If *flags* & ``TCG_BSWAP_OZ``, then *t0* will be zero-extended from bit 15.
       | If *flags* & ``TCG_BSWAP_OS``, then *t0* will be sign-extended from bit 15.
       |
       | If neither ``TCG_BSWAP_OZ`` nor ``TCG_BSWAP_OS`` are set, then the bits of *t0* above bit 15 may contain any value.

   * - bswap32_i64 *t0*, *t1*, *flags*

     - | 32 bit byte swap on a 64-bit value.  The flags are the same as for bswap16,
         except they apply from bit 31 instead of bit 15.

   * - bswap32_i32 *t0*, *t1*, *flags*

       bswap64_i64 *t0*, *t1*, *flags*

     - | 32/64 bit byte swap. The flags are ignored, but still present
         for consistency with the other bswap opcodes.

In an ideal world we could generate kdoc from the source file and
include it in the rest of the tcg docs. I'm not sure if it worth the
churn though? Richard?

https://qemu.readthedocs.io/en/master/devel/tcg-ops.html

>
> Philippe Mathieu-Daudé (7):
>   tcg/tcg-op: Document bswap16() byte pattern
>   tcg/tcg-op: Document bswap32() byte pattern
>   tcg/tcg-op: Document bswap64() byte pattern
>   tcg/tcg-op: Document hswap() byte pattern
>   tcg/tcg-op: Document wswap() byte pattern
>   tcg/tcg-op: Document deposit_z()
>   target/cris: Fix a typo in gen_swapr()
>
>  docs/devel/tcg-ops.rst  | 12 ++++++
>  target/cris/translate.c | 20 +++++----
>  tcg/tcg-op.c            | 96 +++++++++++++++++++++++++++++++----------
>  3 files changed, 96 insertions(+), 32 deletions(-)


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/7] tcg: Document *swap/deposit helpers

[Peter Maydell]

On Tue, 22 Aug 2023 at 14:46, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > While reviewing a recent patch from Richard optimizing
> > deposit() [*] I ended looking at the *swap friends, taking
> > some notes, which then evolved to proper documentation.
> >
> > [*]
> > https://lore.kernel.org/qemu-devel/20230816145547.477974-3-richard.henderson@linaro.org/
>
> We already have some documentation in tcg.rst:
>
>    * - bswap16_i32/i64 *t0*, *t1*, *flags*
>
>      - | 16 bit byte swap on the low bits of a 32/64 bit input.
>        |
>        | If *flags* & ``TCG_BSWAP_IZ``, then *t1* is known to be zero-extended from bit 15.
>        | If *flags* & ``TCG_BSWAP_OZ``, then *t0* will be zero-extended from bit 15.
>        | If *flags* & ``TCG_BSWAP_OS``, then *t0* will be sign-extended from bit 15.
>        |
>        | If neither ``TCG_BSWAP_OZ`` nor ``TCG_BSWAP_OS`` are set, then the bits of *t0* above bit 15 may contain any value.
>
>    * - bswap32_i64 *t0*, *t1*, *flags*
>
>      - | 32 bit byte swap on a 64-bit value.  The flags are the same as for bswap16,
>          except they apply from bit 31 instead of bit 15.
>
>    * - bswap32_i32 *t0*, *t1*, *flags*
>
>        bswap64_i64 *t0*, *t1*, *flags*
>
>      - | 32/64 bit byte swap. The flags are ignored, but still present
>          for consistency with the other bswap opcodes.
>
> In an ideal world we could generate kdoc from the source file and
> include it in the rest of the tcg docs. I'm not sure if it worth the
> churn though? Richard?

I do think it would be useful to have documentation of the
set of APIs you use as a writer of a TCG frontend. This is
often not exactly the same as the TCG IR opcodes. (Similarly
what you have to do as a backend isn't exactly the same,
but the documentation need is less pressing because fewer
people need to work on the backends.)

thanks
-- PMM

Re: [PATCH 0/7] tcg: Document *swap/deposit helpers

[Philippe Mathieu-Daudé]

On 22/8/23 15:42, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> While reviewing a recent patch from Richard optimizing
>> deposit() [*] I ended looking at the *swap friends, taking
>> some notes, which then evolved to proper documentation.
>>
>> [*]
>> https://lore.kernel.org/qemu-devel/20230816145547.477974-3-richard.henderson@linaro.org/
> 
> We already have some documentation in tcg.rst:
> 
>     * - bswap16_i32/i64 *t0*, *t1*, *flags*
> 
>       - | 16 bit byte swap on the low bits of a 32/64 bit input.
>         |
>         | If *flags* & ``TCG_BSWAP_IZ``, then *t1* is known to be zero-extended from bit 15.
>         | If *flags* & ``TCG_BSWAP_OZ``, then *t0* will be zero-extended from bit 15.
>         | If *flags* & ``TCG_BSWAP_OS``, then *t0* will be sign-extended from bit 15.
>         |
>         | If neither ``TCG_BSWAP_OZ`` nor ``TCG_BSWAP_OS`` are set, then the bits of *t0* above bit 15 may contain any value.
> 
>     * - bswap32_i64 *t0*, *t1*, *flags*
> 
>       - | 32 bit byte swap on a 64-bit value.  The flags are the same as for bswap16,
>           except they apply from bit 31 instead of bit 15.
> 
>     * - bswap32_i32 *t0*, *t1*, *flags*
> 
>         bswap64_i64 *t0*, *t1*, *flags*
> 
>       - | 32/64 bit byte swap. The flags are ignored, but still present
>           for consistency with the other bswap opcodes.

I guess I wasn't clear enough: I mostly documented the implementation,
not the API.

That said, maybe the bytes movement pattern belong to the API doc
(at least I find it very useful to find which TCG opcode implement
the frontend code, when the TCG opcode name isn't trivial, although
documented).