diff mbox series

[4/4] target/mips: Fix insert.<b|h|w> for MIPS big endian host

Message ID 1553270080-7829-5-git-send-email-mateja.marjanovic@rt-rk.com (mailing list archive)
State New, archived
Headers show
Series target/mips: Add support for MSA instructions on a big endian host | expand

Commit Message

Mateja Marjanovic March 22, 2019, 3:54 p.m. UTC 
From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

Inserting from GPR to an element in a MSA register when
executed on a MIPS big endian CPU, didn't pick the
right element, and was behaving like on little endian.

Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
---
 target/mips/msa_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Aleksandar Markovic March 22, 2019, 5:16 p.m. UTC | #1 
> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH 4/4] target/mips: Fix insert.<b|h|w> for MIPS big endian host
> 
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> Inserting from GPR to an element in a MSA register when
> executed on a MIPS big endian CPU, didn't pick the
> right element, and was behaving like on little endian.
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---

...

> @@ -1511,9 +1518,11 @@ void helper_msa_insert_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
>      case DF_WORD:
>          pwd->w[n] = (int32_t)rs;
>          break;
> +#ifdef TARGET_MIPS64
>      case DF_DOUBLE:
>          pwd->d[n] = (int64_t)rs;
>          break;
> +#endif
>      default:
>          assert(0);
>      }

You are right that this case should be under ifdef the way you did.

In fact, this code should be impossible to reach, since there is a check
for MIPS32/64 in translate.c before invoking this helper, so technically
there is no bug. However, it is a latent bag, and also an instance of
"dead code" (for MIPS32). So, you are rightfully removing this case
for MIPS32.

May I just ask you to put this in a separate patch, since this has nothing
to do with endianess etc. (with the title, let's say:

"target/mips: Remove handling of nonexistent flavor of INSERT for MIPS32",

and the commit message

"INSERT.D is present in MIPS64 MSA only. [1] page <XXX>

[1] <insert here the latest MSA MIPS64 doc>"

)?

Thanks,
Aleksandar
Mateja Marjanovic March 22, 2019, 5:27 p.m. UTC | #2 
On 22.3.19. 18:16, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH 4/4] target/mips: Fix insert.<b|h|w> for MIPS big endian host
>>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Inserting from GPR to an element in a MSA register when
>> executed on a MIPS big endian CPU, didn't pick the
>> right element, and was behaving like on little endian.
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> ---
> ...
>
>> @@ -1511,9 +1518,11 @@ void helper_msa_insert_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
>>       case DF_WORD:
>>           pwd->w[n] = (int32_t)rs;
>>           break;
>> +#ifdef TARGET_MIPS64
>>       case DF_DOUBLE:
>>           pwd->d[n] = (int64_t)rs;
>>           break;
>> +#endif
>>       default:
>>           assert(0);
>>       }
> You are right that this case should be under ifdef the way you did.
>
> In fact, this code should be impossible to reach, since there is a check
> for MIPS32/64 in translate.c before invoking this helper, so technically
> there is no bug. However, it is a latent bag, and also an instance of
> "dead code" (for MIPS32). So, you are rightfully removing this case
> for MIPS32.
>
> May I just ask you to put this in a separate patch, since this has nothing
> to do with endianess etc. (with the title, let's say:
>
> "target/mips: Remove handling of nonexistent flavor of INSERT for MIPS32",
>
> and the commit message
>
> "INSERT.D is present in MIPS64 MSA only. [1] page <XXX>
>
> [1] <insert here the latest MSA MIPS64 doc>"
>
> )?
You are right, it has nothing to do with the endianness problem. I will 
remove that in v2, and add another patch for that.
>
> Thanks,
> Aleksandar
Thanks,
Mateja
Aleksandar Markovic March 22, 2019, 6:04 p.m. UTC | #3 
> >> @@ -1511,9 +1518,11 @@ void helper_msa_insert_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
> >>       case DF_WORD:
> >>           pwd->w[n] = (int32_t)rs;
> >>           break;
> >> +#ifdef TARGET_MIPS64
> >>       case DF_DOUBLE:
> >>           pwd->d[n] = (int64_t)rs;
> >>           break;
> >> +#endif
> >>       default:
> >>           assert(0);
> >>       }
> > You are right that this case should be under ifdef the way you did.
> >
> > In fact, this code should be impossible to reach, since there is a check
> > for MIPS32/64 in translate.c before invoking this helper, so technically
> > there is no bug. However, it is a latent bag, and also an instance of
> > "dead code" (for MIPS32). So, you are rightfully removing this case
> > for MIPS32.
> >
> > May I just ask you to put this in a separate patch, since this has nothing
> > to do with endianess etc. (with the title, let's say:
> >
> > "target/mips: Remove handling of nonexistent flavor of INSERT for MIPS32",
> >
> > and the commit message
> >
> > "INSERT.D is present in MIPS64 MSA only. [1] page <XXX>
> >
> > [1] <insert here the latest MSA MIPS64 doc>"
> >
> > )?
> You are right, it has nothing to do with the endianness problem. I will
> remove that in v2, and add another patch for that.


Mateja, I noticed that FILL.D has almost identical problem.

Could you create a separate patch for FILL.D too, dealing with the same
issue?

And also analyze all MIPS64-only MSA instructions in this regard?

Glancing at the MIPS64 MSA doc, they are:

COPY_S.D
COPY_U.W (this is not a typo, it is W, not D)
FILL.D
INSERT.D

Yours,
Aleksandar

> >
> > Thanks,
> > Aleksandar
> Thanks,
> Mateja
Mateja Marjanovic March 22, 2019, 6:40 p.m. UTC | #4 
On 22.3.19. 19:04, Aleksandar Markovic wrote:
>>>> @@ -1511,9 +1518,11 @@ void helper_msa_insert_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
>>>>        case DF_WORD:
>>>>            pwd->w[n] = (int32_t)rs;
>>>>            break;
>>>> +#ifdef TARGET_MIPS64
>>>>        case DF_DOUBLE:
>>>>            pwd->d[n] = (int64_t)rs;
>>>>            break;
>>>> +#endif
>>>>        default:
>>>>            assert(0);
>>>>        }
>>> You are right that this case should be under ifdef the way you did.
>>>
>>> In fact, this code should be impossible to reach, since there is a check
>>> for MIPS32/64 in translate.c before invoking this helper, so technically
>>> there is no bug. However, it is a latent bag, and also an instance of
>>> "dead code" (for MIPS32). So, you are rightfully removing this case
>>> for MIPS32.
>>>
>>> May I just ask you to put this in a separate patch, since this has nothing
>>> to do with endianess etc. (with the title, let's say:
>>>
>>> "target/mips: Remove handling of nonexistent flavor of INSERT for MIPS32",
>>>
>>> and the commit message
>>>
>>> "INSERT.D is present in MIPS64 MSA only. [1] page <XXX>
>>>
>>> [1] <insert here the latest MSA MIPS64 doc>"
>>>
>>> )?
>> You are right, it has nothing to do with the endianness problem. I will
>> remove that in v2, and add another patch for that.
>
> Mateja, I noticed that FILL.D has almost identical problem.
>
> Could you create a separate patch for FILL.D too, dealing with the same
> issue?
>
> And also analyze all MIPS64-only MSA instructions in this regard?
>
> Glancing at the MIPS64 MSA doc, they are:
>
> COPY_S.D
> COPY_U.W (this is not a typo, it is W, not D)
> FILL.D
> INSERT.D
Will do.
>
> Yours,
> Aleksandar
Regards,
Mateja
>>> Thanks,
>>> Aleksandar
>> Thanks,
>> Mateja
diff mbox series

Patch

diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
index 8caf186..0049191 100644
--- a/target/mips/msa_helper.c
+++ b/target/mips/msa_helper.c
@@ -1500,6 +1500,13 @@  void helper_msa_insert_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
 {
     wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
     target_ulong rs = env->active_tc.gpr[rs_num];
+#if defined(HOST_WORDS_BIGENDIAN)
+    if (n < DF_ELEMENTS(df) / 2) {
+        n = DF_ELEMENTS(df) / 2 - n - 1;
+    } else {
+        n = 3 * DF_ELEMENTS(df) / 2 - n - 1;
+    }
+#endif
 
     switch (df) {
     case DF_BYTE:
@@ -1511,9 +1518,11 @@  void helper_msa_insert_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
     case DF_WORD:
         pwd->w[n] = (int32_t)rs;
         break;
+#ifdef TARGET_MIPS64
     case DF_DOUBLE:
         pwd->d[n] = (int64_t)rs;
         break;
+#endif
     default:
         assert(0);
     }