[PATCH 1/2] target/mips: Don't check COP1X for 64 bit FP mode

[PATCH 1/2] target/mips: Don't check COP1X for 64 bit FP mode

[Jiaxun Yang]

Some implementations (i.e. Loongson-2F) may decide to implement a 64 bit
FPU without implmenting COP1X instructions.

As the eligibility of 64 bit FP instructions is already determined by
CP0St_FR, there is no need to check for COP1X again.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 target/mips/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 2f2d707a12..e49d2a25a8 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -1545,7 +1545,7 @@ void check_cop1x(DisasContext *ctx)
  */
 void check_cp1_64bitmode(DisasContext *ctx)
 {
-    if (unlikely(~ctx->hflags & (MIPS_HFLAG_F64 | MIPS_HFLAG_COP1X))) {
+    if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
         gen_reserved_instruction(ctx);
     }
 }
-- 
2.34.1

[PATCH 2/2] target/mips: Correct check for CABS instructions

[Jiaxun Yang]

Accroading to "MIPS Architecture for Programmers Volume IV-c:
The MIPS-3D Application-Specific Extension to the MIPS64 Architecture"
(MD00099). CABS.cond.fmt belongs to MIPS-3D ASE, and it has nothing to do
with COP1X opcode.

Remove all unnecessary COP1X checks and check for MIPS3D availability
in decoding code path.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 target/mips/tcg/translate.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index e49d2a25a8..23e575ad95 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -1788,16 +1788,8 @@ static inline void gen_cmp ## type ## _ ## fmt(DisasContext *ctx, int n,      \
         check_ps(ctx);                                                        \
         break;                                                                \
     case FMT_D:                                                               \
-        if (abs) {                                                            \
-            check_cop1x(ctx);                                                 \
-        }                                                                     \
         check_cp1_registers(ctx, fs | ft);                                    \
         break;                                                                \
-    case FMT_S:                                                               \
-        if (abs) {                                                            \
-            check_cop1x(ctx);                                                 \
-        }                                                                     \
-        break;                                                                \
     }                                                                         \
     gen_ldcmp_fpr##bits(ctx, fp0, fs);                                        \
     gen_ldcmp_fpr##bits(ctx, fp1, ft);                                        \
@@ -10424,6 +10416,7 @@ static void gen_farith(DisasContext *ctx, enum fopcode op1,
     case OPC_CMP_NGT_S:
         check_insn_opc_removed(ctx, ISA_MIPS_R6);
         if (ctx->opcode & (1 << 6)) {
+            check_insn(ctx, ASE_MIPS3D);
             gen_cmpabs_s(ctx, func - 48, ft, fs, cc);
         } else {
             gen_cmp_s(ctx, func - 48, ft, fs, cc);
-- 
2.34.1

Re: [PATCH 2/2] target/mips: Correct check for CABS instructions

[Philippe Mathieu-Daudé]

On 2/11/22 17:57, Jiaxun Yang wrote:
> Accroading to "MIPS Architecture for Programmers Volume IV-c:
> The MIPS-3D Application-Specific Extension to the MIPS64 Architecture"
> (MD00099). CABS.cond.fmt belongs to MIPS-3D ASE, and it has nothing to do
> with COP1X opcode.
> 
> Remove all unnecessary COP1X checks and check for MIPS3D availability
> in decoding code path.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   target/mips/tcg/translate.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> index e49d2a25a8..23e575ad95 100644
> --- a/target/mips/tcg/translate.c
> +++ b/target/mips/tcg/translate.c
> @@ -1788,16 +1788,8 @@ static inline void gen_cmp ## type ## _ ## fmt(DisasContext *ctx, int n,      \
>           check_ps(ctx);                                                        \
>           break;                                                                \
>       case FMT_D:                                                               \
> -        if (abs) {                                                            \
> -            check_cop1x(ctx);                                                 \
> -        }                                                                     \
>           check_cp1_registers(ctx, fs | ft);                                    \
>           break;                                                                \
> -    case FMT_S:                                                               \
> -        if (abs) {                                                            \
> -            check_cop1x(ctx);                                                 \
> -        }                                                                     \
> -        break;                                                                \

I'm not sure we want to remove this check on all opcodes handled by
the FOP_CONDS() macro, and for all architecture variants. Maybe we
need to special-case CABS.cond.fmt?

>       }                                                                         \
>       gen_ldcmp_fpr##bits(ctx, fp0, fs);                                        \
>       gen_ldcmp_fpr##bits(ctx, fp1, ft);                                        \
> @@ -10424,6 +10416,7 @@ static void gen_farith(DisasContext *ctx, enum fopcode op1,
>       case OPC_CMP_NGT_S:
>           check_insn_opc_removed(ctx, ISA_MIPS_R6);
>           if (ctx->opcode & (1 << 6)) {
> +            check_insn(ctx, ASE_MIPS3D);

You somehow revert commit b8aa4598e2 ("MIPS COP1X (and related)
instructions") which is in use since 15 years.

>               gen_cmpabs_s(ctx, func - 48, ft, fs, cc);
>           } else {
>               gen_cmp_s(ctx, func - 48, ft, fs, cc);

Re: [PATCH 1/2] target/mips: Don't check COP1X for 64 bit FP mode

[Philippe Mathieu-Daudé]

On 2/11/22 17:57, Jiaxun Yang wrote:
> Some implementations (i.e. Loongson-2F) may decide to implement a 64 bit
> FPU without implmenting COP1X instructions.
> 
> As the eligibility of 64 bit FP instructions is already determined by
> CP0St_FR, there is no need to check for COP1X again.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   target/mips/tcg/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> index 2f2d707a12..e49d2a25a8 100644
> --- a/target/mips/tcg/translate.c
> +++ b/target/mips/tcg/translate.c
> @@ -1545,7 +1545,7 @@ void check_cop1x(DisasContext *ctx)
>    */
>   void check_cp1_64bitmode(DisasContext *ctx)
>   {
> -    if (unlikely(~ctx->hflags & (MIPS_HFLAG_F64 | MIPS_HFLAG_COP1X))) {
> +    if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
>           gen_reserved_instruction(ctx);
>       }
>   }

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

Re: [PATCH 1/2] target/mips: Don't check COP1X for 64 bit FP mode

[Philippe Mathieu-Daudé]

On 2/11/22 17:57, Jiaxun Yang wrote:
> Some implementations (i.e. Loongson-2F) may decide to implement a 64 bit
> FPU without implmenting COP1X instructions.
> 
> As the eligibility of 64 bit FP instructions is already determined by
> CP0St_FR, there is no need to check for COP1X again.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   target/mips/tcg/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Queued to mips-fixes, thanks.

Re: [PATCH 1/2] target/mips: Don't check COP1X for 64 bit FP mode

[Philippe Mathieu-Daudé]

On 7/11/22 23:47, Philippe Mathieu-Daudé wrote:
> On 2/11/22 17:57, Jiaxun Yang wrote:
>> Some implementations (i.e. Loongson-2F) may decide to implement a 64 bit
>> FPU without implmenting COP1X instructions.
>>
>> As the eligibility of 64 bit FP instructions is already determined by
>> CP0St_FR, there is no need to check for COP1X again.
>>
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>>   target/mips/tcg/translate.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
>> index 2f2d707a12..e49d2a25a8 100644
>> --- a/target/mips/tcg/translate.c
>> +++ b/target/mips/tcg/translate.c
>> @@ -1545,7 +1545,7 @@ void check_cop1x(DisasContext *ctx)
>>    */
>>   void check_cp1_64bitmode(DisasContext *ctx)
>>   {
>> -    if (unlikely(~ctx->hflags & (MIPS_HFLAG_F64 | MIPS_HFLAG_COP1X))) {
>> +    if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
>>           gen_reserved_instruction(ctx);
>>       }
>>   }

Did you test your patch? I'm getting:

../../target/mips/tcg/translate.c:1548:49: error: expected ')'
     if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
                                                 ^
../../target/mips/tcg/translate.c:1548:8: note: to match this '('
     if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
        ^
../../target/mips/tcg/translate.c:1551:1: error: expected statement
}
^

Re: [PATCH 2/2] target/mips: Correct check for CABS instructions

[Jiaxun Yang]

在 2022/11/7 22:35, Philippe Mathieu-Daudé 写道:
> On 2/11/22 17:57, Jiaxun Yang wrote:
>> Accroading to "MIPS Architecture for Programmers Volume IV-c:
>> The MIPS-3D Application-Specific Extension to the MIPS64 Architecture"
>> (MD00099). CABS.cond.fmt belongs to MIPS-3D ASE, and it has nothing 
>> to do
>> with COP1X opcode.
>>
>> Remove all unnecessary COP1X checks and check for MIPS3D availability
>> in decoding code path.
>>
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> ---
>>   target/mips/tcg/translate.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
>> index e49d2a25a8..23e575ad95 100644
>> --- a/target/mips/tcg/translate.c
>> +++ b/target/mips/tcg/translate.c
>> @@ -1788,16 +1788,8 @@ static inline void gen_cmp ## type ## _ ## 
>> fmt(DisasContext *ctx, int n,      \
>> check_ps(ctx); \
>> break; \
>>       case FMT_D: \
>> -        if (abs) 
>> {                                                            \
>> - check_cop1x(ctx); \
>> - } \
>>           check_cp1_registers(ctx, fs | 
>> ft);                                    \
>> break; \
>> -    case FMT_S: \
>> -        if (abs) 
>> {                                                            \
>> - check_cop1x(ctx); \
>> - } \
>> - break; \
>
> I'm not sure we want to remove this check on all opcodes handled by
> the FOP_CONDS() macro, and for all architecture variants. Maybe we
> need to special-case CABS.cond.fmt?

Hmm if I read the code correctly COP1X check is only ran when abs is
set, and the only case abs is set is for CABS.cond.fmt.

>
>> } \
>>       gen_ldcmp_fpr##bits(ctx, fp0, 
>> fs);                                        \
>>       gen_ldcmp_fpr##bits(ctx, fp1, 
>> ft);                                        \
>> @@ -10424,6 +10416,7 @@ static void gen_farith(DisasContext *ctx, 
>> enum fopcode op1,
>>       case OPC_CMP_NGT_S:
>>           check_insn_opc_removed(ctx, ISA_MIPS_R6);
>>           if (ctx->opcode & (1 << 6)) {
>> +            check_insn(ctx, ASE_MIPS3D);
>
> You somehow revert commit b8aa4598e2 ("MIPS COP1X (and related)
> instructions") which is in use since 15 years.

Still no idea about why it is here in first place....
CABS.cond.fmt is even not mentioned in MIPS IV manual. So it's unlikely 
to have
anything to do with COP1X.

Thanks
- Jiaxun

>
>>               gen_cmpabs_s(ctx, func - 48, ft, fs, cc);
>>           } else {
>>               gen_cmp_s(ctx, func - 48, ft, fs, cc);
>

Re: [PATCH 1/2] target/mips: Don't check COP1X for 64 bit FP mode

[Jiaxun Yang]

在 2022/11/7 23:29, Philippe Mathieu-Daudé 写道:
> On 7/11/22 23:47, Philippe Mathieu-Daudé wrote:
>> On 2/11/22 17:57, Jiaxun Yang wrote:
>>> Some implementations (i.e. Loongson-2F) may decide to implement a 64 
>>> bit
>>> FPU without implmenting COP1X instructions.
>>>
>>> As the eligibility of 64 bit FP instructions is already determined by
>>> CP0St_FR, there is no need to check for COP1X again.
>>>
>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> ---
>>>   target/mips/tcg/translate.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
>>> index 2f2d707a12..e49d2a25a8 100644
>>> --- a/target/mips/tcg/translate.c
>>> +++ b/target/mips/tcg/translate.c
>>> @@ -1545,7 +1545,7 @@ void check_cop1x(DisasContext *ctx)
>>>    */
>>>   void check_cp1_64bitmode(DisasContext *ctx)
>>>   {
>>> -    if (unlikely(~ctx->hflags & (MIPS_HFLAG_F64 | 
>>> MIPS_HFLAG_COP1X))) {
>>> +    if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
>>>           gen_reserved_instruction(ctx);
>>>       }
>>>   }
>
> Did you test your patch? I'm getting:
>
> ../../target/mips/tcg/translate.c:1548:49: error: expected ')'
>     if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
>                                                 ^
> ../../target/mips/tcg/translate.c:1548:8: note: to match this '('
>     if (unlikely(~ctx->hflags & MIPS_HFLAG_F64) {
>        ^
> ../../target/mips/tcg/translate.c:1551:1: error: expected statement
> }

Woah, typo when copy changes back from test machine...
Will use git publish next time.

Thanks
- Jiaxun


> ^
>