Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

[PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

Posted by Alex Richardson 3 years, 9 months ago

After merging latest QEMU upstream into our CHERI fork, I noticed that
some of the FPU tests in our MIPS baremetal testsuite
(https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
this commit accidentally changed add.s into a subtract.

Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
---
 target/mips/fpu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
index 7a3a61cab3..56beda49d8 100644
--- a/target/mips/fpu_helper.c
+++ b/target/mips/fpu_helper.c
@@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
 {
     uint32_t wt2;
 
-    wt2 = float32_sub(fst0, fst1, &env->active_fpu.fp_status);
+    wt2 = float32_add(fst0, fst1, &env->active_fpu.fp_status);
     update_fcr31(env, GETPC());
     return wt2;
 }
-- 
2.27.0

Re: [PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

Posted by Aleksandar Markovic 3 years, 9 months ago

On Friday, July 3, 2020, Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
wrote:

> After merging latest QEMU upstream into our CHERI fork, I noticed that
> some of the FPU tests in our MIPS baremetal testsuite
> (https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
> this commit accidentally changed add.s into a subtract.
>
>
Alex, all you said sounds very probable to me. I currently don't have any
dev system, at hand, but as soon as I get it, I will confirm/disconfirm
(but again, most likely confirm) this problem, and than we'll give you
green lifght for your patch.

Yours,
Aleksandar




> Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
> ---
>  target/mips/fpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
> index 7a3a61cab3..56beda49d8 100644
> --- a/target/mips/fpu_helper.c
> +++ b/target/mips/fpu_helper.c
> @@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
>  {
>      uint32_t wt2;
>
> -    wt2 = float32_sub(fst0, fst1, &env->active_fpu.fp_status);
> +    wt2 = float32_add(fst0, fst1, &env->active_fpu.fp_status);
>      update_fcr31(env, GETPC());
>      return wt2;
>  }
> --
> 2.27.0
>
>

Re: [PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

Posted by Aleksandar Markovic 3 years, 9 months ago

On Fri, Jul 3, 2020 at 6:33 PM Alex Richardson
<Alexander.Richardson@cl.cam.ac.uk> wrote:
>
> After merging latest QEMU upstream into our CHERI fork, I noticed that
> some of the FPU tests in our MIPS baremetal testsuite
> (https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
> this commit accidentally changed add.s into a subtract.
>
> Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
> ---

Applied to MIPS + TCG Continuous Benchmarking queue.

>  target/mips/fpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
> index 7a3a61cab3..56beda49d8 100644
> --- a/target/mips/fpu_helper.c
> +++ b/target/mips/fpu_helper.c
> @@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
>  {
>      uint32_t wt2;
>
> -    wt2 = float32_sub(fst0, fst1, &env->active_fpu.fp_status);
> +    wt2 = float32_add(fst0, fst1, &env->active_fpu.fp_status);
>      update_fcr31(env, GETPC());
>      return wt2;
>  }
> --
> 2.27.0
>
>

Re: [PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

Hi Aleksandar,

On 7/7/20 6:26 PM, Aleksandar Markovic wrote:
> On Fri, Jul 3, 2020 at 6:33 PM Alex Richardson
> <Alexander.Richardson@cl.cam.ac.uk> wrote:
>>
>> After merging latest QEMU upstream into our CHERI fork, I noticed that
>> some of the FPU tests in our MIPS baremetal testsuite
>> (https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
>> this commit accidentally changed add.s into a subtract.
>>
>> Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
>> ---
> 
> Applied to MIPS + TCG Continuous Benchmarking queue.

If you don't mind I'll include this patch for the mips pull request I
plan to send before hard freeze (on the list). I'm keeping your S-o-b.

> 
>>  target/mips/fpu_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
>> index 7a3a61cab3..56beda49d8 100644
>> --- a/target/mips/fpu_helper.c
>> +++ b/target/mips/fpu_helper.c
>> @@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
>>  {
>>      uint32_t wt2;
>>
>> -    wt2 = float32_sub(fst0, fst1, &env->active_fpu.fp_status);
>> +    wt2 = float32_add(fst0, fst1, &env->active_fpu.fp_status);
>>      update_fcr31(env, GETPC());
>>      return wt2;
>>  }
>> --
>> 2.27.0
>>
>>
>

Re: [PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

Hi Alex,

On 7/3/20 6:15 PM, Alex Richardson wrote:
> After merging latest QEMU upstream into our CHERI fork, I noticed that
> some of the FPU tests in our MIPS baremetal testsuite

I understand by baremetal your soft core implementation running on
a FPGA, right?

> (https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
> this commit accidentally changed add.s into a subtract.

Fixes: 1ace099f2a ("target/mips: fpu: Demacro ADD.<D|S|PS>")
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for your quick fix!

Phil.

> Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
> ---
>  target/mips/fpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
> index 7a3a61cab3..56beda49d8 100644
> --- a/target/mips/fpu_helper.c
> +++ b/target/mips/fpu_helper.c
> @@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
>  {
>      uint32_t wt2;
>  
> -    wt2 = float32_sub(fst0, fst1, &env->active_fpu.fp_status);
> +    wt2 = float32_add(fst0, fst1, &env->active_fpu.fp_status);
>      update_fcr31(env, GETPC());
>      return wt2;
>  }
>

Re: [PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

Posted by Alexander Richardson 3 years, 9 months ago

Hi Philippe,

On Fri, 3 Jul 2020 at 19:40, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Alex,
>
> On 7/3/20 6:15 PM, Alex Richardson wrote:
> > After merging latest QEMU upstream into our CHERI fork, I noticed that
> > some of the FPU tests in our MIPS baremetal testsuite
>
> I understand by baremetal your soft core implementation running on
> a FPGA, right?
>
By baremetal I mean small test binaries running without a host OS.
The test suite was originally written for the CHERI FPGAs, but it also
supports various MIPS simulators, our executable formal model written
in sail and our fork of QEMU (https://github.com/CTSRD-CHERI/qemu).
Unfortunately it cannot be run with upstream QEMU as it requires a
special MTC0 instruction to dump register values in a textual format
to the logfile.

> > (https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
> > this commit accidentally changed add.s into a subtract.
>
> Fixes: 1ace099f2a ("target/mips: fpu: Demacro ADD.<D|S|PS>")
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Thanks for your quick fix!
>
> Phil.
>
> > Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
> > ---
> >  target/mips/fpu_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
> > index 7a3a61cab3..56beda49d8 100644
> > --- a/target/mips/fpu_helper.c
> > +++ b/target/mips/fpu_helper.c
> > @@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
> >  {
> >      uint32_t wt2;
> >
> > -    wt2 = float32_sub(fst0, fst1, &env->active_fpu.fp_status);
> > +    wt2 = float32_add(fst0, fst1, &env->active_fpu.fp_status);
> >      update_fcr31(env, GETPC());
> >      return wt2;
> >  }
> >
>