target/mips: Fix DBALIGN DSP-R2 opcode 'byte position' field size

[PATCH] target/mips: Fix DBALIGN DSP-R2 opcode 'byte position' field size

Posted by Philippe Mathieu-Daudé 2 years, 11 months ago

Per the "MIPS® DSP Module for MIPS64 Architecture" manual (rev 3.02),
Figure 5.12 "SPECIAL3 Encoding of APPEND/DAPPEND Instruction Sub-class"
the byte position field ('bp') is 2 bits, not 3.

Cc: Jia Liu <proljc@gmail.com>
Fixes: 26690560240 ("target-mips: Add ASE DSP compare-pick instructions")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/tcg/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index c03a8ae1fed..e68647ce14c 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -23016,8 +23016,8 @@ static void gen_mipsdsp_append(CPUMIPSState *env, DisasContext *ctx,
             }
             break;
         case OPC_DBALIGN:
-            sa &= 7;
-            if (sa != 0 && sa != 2 && sa != 4) {
+            sa &= 3;
+            if (sa != 0 && sa != 2) {
                 tcg_gen_shli_tl(cpu_gpr[rt], cpu_gpr[rt], 8 * sa);
                 tcg_gen_shri_tl(t0, t0, 8 * (8 - sa));
                 tcg_gen_or_tl(cpu_gpr[rt], cpu_gpr[rt], t0);
-- 
2.26.3

Re: [PATCH] target/mips: Fix DBALIGN DSP-R2 opcode 'byte position' field size

Posted by Philippe Mathieu-Daudé 2 years, 11 months ago

On 5/29/21 3:05 PM, Philippe Mathieu-Daudé wrote:
> Per the "MIPS® DSP Module for MIPS64 Architecture" manual (rev 3.02),
> Figure 5.12 "SPECIAL3 Encoding of APPEND/DAPPEND Instruction Sub-class"
> the byte position field ('bp') is 2 bits, not 3.
> 
> Cc: Jia Liu <proljc@gmail.com>
> Fixes: 26690560240 ("target-mips: Add ASE DSP compare-pick instructions")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/tcg/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> index c03a8ae1fed..e68647ce14c 100644
> --- a/target/mips/tcg/translate.c
> +++ b/target/mips/tcg/translate.c
> @@ -23016,8 +23016,8 @@ static void gen_mipsdsp_append(CPUMIPSState *env, DisasContext *ctx,
>              }
>              break;
>          case OPC_DBALIGN:
> -            sa &= 7;
> -            if (sa != 0 && sa != 2 && sa != 4) {
> +            sa &= 3;
> +            if (sa != 0 && sa != 2) {
>                  tcg_gen_shli_tl(cpu_gpr[rt], cpu_gpr[rt], 8 * sa);
>                  tcg_gen_shri_tl(t0, t0, 8 * (8 - sa));
>                  tcg_gen_or_tl(cpu_gpr[rt], cpu_gpr[rt], t0);
> 

Looking at GCC, there is a patch adding this opcode:
https://gcc.gnu.org/legacy-ml/gcc-patches/2012-02/msg00127.html
which uses 2 bits:

+(define_insn "mips_dbalign"
+  [(set (match_operand:DI 0 "register_operand" "=d")
+	(unspec:DI [(match_operand:DI 1 "register_operand" "0")
+		    (match_operand:DI 2 "reg_or_0_operand" "dJ")
+		    (match_operand:DI 3 "const_int_operand" "n")]
+		   UNSPEC_DBALIGN))]
+  "ISA_HAS_DSPR2"
+{
+  if (INTVAL (operands[3]) & ~(unsigned HOST_WIDE_INT) 3)
+    operands[2] = GEN_INT (INTVAL (operands[2]) & 3);
+  return "dbalign\t%0,%z2,%3";
+}
+  [(set_attr "type"	"arith")
+   (set_attr "mode"	"DI")])

However looking at the releases/gcc-11.1.0 tag it seems GCC never
supported DBALIGN...:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/mips/mips-dspr2.md;h=95ba712f6a12946100cd4fe98d05732e70de8f98;hb=50bc9185c2821350f0b785d6e23a6e9dcde58466

Re: [PATCH] target/mips: Fix DBALIGN DSP-R2 opcode 'byte position' field size

Posted by Richard Henderson 2 years, 11 months ago

On 5/29/21 6:05 AM, Philippe Mathieu-Daudé wrote:
> Per the "MIPS® DSP Module for MIPS64 Architecture" manual (rev 3.02),
> Figure 5.12 "SPECIAL3 Encoding of APPEND/DAPPEND Instruction Sub-class"
> the byte position field ('bp') is 2 bits, not 3.

Rev 2.34 has 3 bits, not 2.

The mips32 version of balign, that uses 2 bits...  Are you sure you looked at 
the right instruction?  Because 3 bits makes most sense for this instruction 
with a 64-bit register size.


r~

> 
> Cc: Jia Liu <proljc@gmail.com>
> Fixes: 26690560240 ("target-mips: Add ASE DSP compare-pick instructions")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   target/mips/tcg/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> index c03a8ae1fed..e68647ce14c 100644
> --- a/target/mips/tcg/translate.c
> +++ b/target/mips/tcg/translate.c
> @@ -23016,8 +23016,8 @@ static void gen_mipsdsp_append(CPUMIPSState *env, DisasContext *ctx,
>               }
>               break;
>           case OPC_DBALIGN:
> -            sa &= 7;
> -            if (sa != 0 && sa != 2 && sa != 4) {
> +            sa &= 3;
> +            if (sa != 0 && sa != 2) {
>                   tcg_gen_shli_tl(cpu_gpr[rt], cpu_gpr[rt], 8 * sa);
>                   tcg_gen_shri_tl(t0, t0, 8 * (8 - sa));
>                   tcg_gen_or_tl(cpu_gpr[rt], cpu_gpr[rt], t0);
>

Re: [PATCH] target/mips: Fix DBALIGN DSP-R2 opcode 'byte position' field size

Posted by Philippe Mathieu-Daudé 2 years, 10 months ago

On 5/30/21 5:33 PM, Richard Henderson wrote:
> On 5/29/21 6:05 AM, Philippe Mathieu-Daudé wrote:
>> Per the "MIPS® DSP Module for MIPS64 Architecture" manual (rev 3.02),
>> Figure 5.12 "SPECIAL3 Encoding of APPEND/DAPPEND Instruction Sub-class"
>> the byte position field ('bp') is 2 bits, not 3.
> 
> Rev 2.34 has 3 bits, not 2.
> 
> The mips32 version of balign, that uses 2 bits...  Are you sure you
> looked at the right instruction?  Because 3 bits makes most sense for
> this instruction with a 64-bit register size.

Yes indeed it makes sense, and Rev 3.02 is incomplete...

Thanks,

Phil.