[SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 220 bytes --]

Hi,

On SH variants with double-precision FPU the insns fli0 and flid1 require
that FPSCR.PR must be set to 0, i.e. single-precision mode.  The attached
patch fixes that.

OK to apply?

Best regards,
Oleg Endo

[-- Attachment #2: 0001-simsh-check-PFSCRPR-setting-for-fldi0-and-fldi1-insns.patch --]
[-- Type: text/x-patch, Size: 1096 bytes --]

From 3417353c867efd502da6df1e80b9a8bdb0a422f0 Mon Sep 17 00:00:00 2001
From: Oleg Endo <olegendo@gcc.gnu.org>
Date: Wed, 11 Mar 2026 11:55:20 +0900
Subject: [PATCH] sim/sh: check PFSCR.PR setting for fldi0 and fldi1 insns

---
 sim/sh/gencode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index 42009fc..9c882d2 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -644,18 +644,22 @@ static op tab[] =
 
   /* sh2e */
   { "", "", "fldi0 <FREG_N>", "1111nnnn10001101",
     {
-      "SET_FR (n, (float) 0.0);",
-      "/* FIXME: check for DP and (n & 1) == 0?  */",
+      "if (FPSCR_PR)",
+      "  RAISE_EXCEPTION (SIGILL);",
+      "else",
+      "  SET_FR (n, (float) 0.0);",
     },
   },
 
   /* sh2e */
   { "", "", "fldi1 <FREG_N>", "1111nnnn10011101",
     {
-      "SET_FR (n, (float) 1.0);",
-      "/* FIXME: check for DP and (n & 1) == 0?  */",
+      "if (FPSCR_PR)",
+      "  RAISE_EXCEPTION (SIGILL);",
+      "else",
+      "  SET_FR (n, (float) 1.0);",
     },
   },
 
   /* sh2e */
--
libgit2 1.9.1

Re: [SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Simon Marchi]

On 2026-03-10 23:02, Oleg Endo wrote:
> Hi,
> 
> On SH variants with double-precision FPU the insns fli0 and flid1 require
> that FPSCR.PR must be set to 0, i.e. single-precision mode.  The attached
> patch fixes that.
> 
> OK to apply?
> 
> Best regards,
> Oleg Endo

I looked for an ISA manual [1] and checked it for fun.  For the FLID0,
it says:

  When FPSCR.PR = 0, this instruction loads floating-point 0.0 (0x00000000) into FRn.

Curiously, it does not say what happens if `FPSCR.PR = 1`.  However,
the description for FPSCR.PR says:

  PR: Precision mode
  PR = 0: Floating-point instructions are executed as single-precision operations.
  PR = 1: Floating-point instructions are executed as double-precision operations (the result of
  instructions for which double-precision is not supported is undefined).

I guess that FLID0 and FLID1 fall into that "is undefined" category?
Do you know what the real hardware does in this case?

The patch LGTM, but I am not a maintainer of the sim, so:

Reviewed-By: Simon Marchi <simon.marchi@efficios.com>

One note, please put the relevant information (the body of your email)
into the commit log itself).

Thanks,

Simon

[1] https://0x04.net/~mwk/doc/sh/e602156_sh4.pdf

Re: [SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]

On Wed, 2026-03-11 at 00:27 -0400, Simon Marchi wrote:
> 
> On 2026-03-10 23:02, Oleg Endo wrote:
> > Hi,
> > 
> > On SH variants with double-precision FPU the insns fli0 and flid1 require
> > that FPSCR.PR must be set to 0, i.e. single-precision mode.  The attached
> > patch fixes that.
> > 
> > OK to apply?
> > 
> > Best regards,
> > Oleg Endo
> 
> I looked for an ISA manual [1] and checked it for fun.  For the FLID0,
> it says:
> 
>   When FPSCR.PR = 0, this instruction loads floating-point 0.0 (0x00000000) into FRn.
> 
> Curiously, it does not say what happens if `FPSCR.PR = 1`.  However,
> the description for FPSCR.PR says:
> 
>   PR: Precision mode
>   PR = 0: Floating-point instructions are executed as single-precision operations.
>   PR = 1: Floating-point instructions are executed as double-precision operations (the result of
>   instructions for which double-precision is not supported is undefined).
> 
> I guess that FLID0 and FLID1 fall into that "is undefined" category?
> Do you know what the real hardware does in this case?

I have not checked what exactly happens on real hardware.  I have checked a
few ISA manuals (Renesas, ST).  The instruction descriptions doesn't say
that FLDI0 / FLID1 raises an exception (when in the wrong mode).  So I guess
it will produce garbage quietly.  Maybe some silicon implementations have
actually an undocumented but well defined behavior.

QEMU traps in this case.  We found this while chasing an GCC bug.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117182

I find using sh-sim for compiler testing quite useful.  Hence this patch.



> 
> The patch LGTM, but I am not a maintainer of the sim, so:
> 
> Reviewed-By: Simon Marchi <simon.marchi@efficios.com>
> 
> One note, please put the relevant information (the body of your email)
> into the commit log itself).

Yeah, sure I can expand the comment of the commit.  Updated patch/commit
attached.

Best regards,
Oleg Endo


[-- Attachment #2: 0002-simsh-check-PFSCRPR-setting-for-fldi0-and-fldi1-insns.patch --]
[-- Type: text/x-patch, Size: 1330 bytes --]

From dcda716308837151a22b765381d0003cc4b9723f Mon Sep 17 00:00:00 2001
From: Oleg Endo <olegendo@gcc.gnu.org>
Date: Wed, 11 Mar 2026 11:55:20 +0900
Subject: [PATCH] sim/sh: check PFSCR.PR setting for fldi0 and fldi1 insns

On SH variants with double-precision FPU the insns fli0 and flid1 are only
defined when FPSCR.PR = 0.  The hardware does not necessarily trap but might
quietly load undefined values.  However, qemu traps in that case, so do the
same.
---
 sim/sh/gencode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index 42009fc..9c882d2 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -644,18 +644,22 @@ static op tab[] =
 
   /* sh2e */
   { "", "", "fldi0 <FREG_N>", "1111nnnn10001101",
     {
-      "SET_FR (n, (float) 0.0);",
-      "/* FIXME: check for DP and (n & 1) == 0?  */",
+      "if (FPSCR_PR)",
+      "  RAISE_EXCEPTION (SIGILL);",
+      "else",
+      "  SET_FR (n, (float) 0.0);",
     },
   },
 
   /* sh2e */
   { "", "", "fldi1 <FREG_N>", "1111nnnn10011101",
     {
-      "SET_FR (n, (float) 1.0);",
-      "/* FIXME: check for DP and (n & 1) == 0?  */",
+      "if (FPSCR_PR)",
+      "  RAISE_EXCEPTION (SIGILL);",
+      "else",
+      "  SET_FR (n, (float) 1.0);",
     },
   },
 
   /* sh2e */
--
libgit2 1.9.1

Re: [SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Simon Marchi]

On 3/11/26 2:20 AM, Oleg Endo wrote:
> On Wed, 2026-03-11 at 00:27 -0400, Simon Marchi wrote:
>>
>> On 2026-03-10 23:02, Oleg Endo wrote:
>>> Hi,
>>>
>>> On SH variants with double-precision FPU the insns fli0 and flid1 require
>>> that FPSCR.PR must be set to 0, i.e. single-precision mode.  The attached
>>> patch fixes that.
>>>
>>> OK to apply?
>>>
>>> Best regards,
>>> Oleg Endo
>>
>> I looked for an ISA manual [1] and checked it for fun.  For the FLID0,
>> it says:
>>
>>   When FPSCR.PR = 0, this instruction loads floating-point 0.0 (0x00000000) into FRn.
>>
>> Curiously, it does not say what happens if `FPSCR.PR = 1`.  However,
>> the description for FPSCR.PR says:
>>
>>   PR: Precision mode
>>   PR = 0: Floating-point instructions are executed as single-precision operations.
>>   PR = 1: Floating-point instructions are executed as double-precision operations (the result of
>>   instructions for which double-precision is not supported is undefined).
>>
>> I guess that FLID0 and FLID1 fall into that "is undefined" category?
>> Do you know what the real hardware does in this case?
> 
> I have not checked what exactly happens on real hardware.  I have checked a
> few ISA manuals (Renesas, ST).  The instruction descriptions doesn't say
> that FLDI0 / FLID1 raises an exception (when in the wrong mode).  So I guess
> it will produce garbage quietly.  Maybe some silicon implementations have
> actually an undocumented but well defined behavior.

Ok, well since we're in "undefined behavior" territory we could argue
that the current sim behavior is fine.  But I think the patch is still
good, traping is a more useful behavior to catch something you don't
want to happen.

> 
> QEMU traps in this case.  We found this while chasing an GCC bug.
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117182
> 
> I find using sh-sim for compiler testing quite useful.  Hence this patch.

Good to know, thanks.  We often wonder "does anybody still use some old
architecture or feature X in gdb".

>> The patch LGTM, but I am not a maintainer of the sim, so:
>>
>> Reviewed-By: Simon Marchi <simon.marchi@efficios.com>
>>
>> One note, please put the relevant information (the body of your email)
>> into the commit log itself).
> 
> Yeah, sure I can expand the comment of the commit.  Updated patch/commit
> attached.

I would give it a week for Andrew or Mike to review and approve, but
otherwise I would feel comfortable merging it.

Simon

Re: [SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Oleg Endo]

On Wed, 2026-03-11 at 10:39 -0400, Simon Marchi wrote:
> 
> Ok, well since we're in "undefined behavior" territory we could argue
> that the current sim behavior is fine.  But I think the patch is still
> good, traping is a more useful behavior to catch something you don't
> want to happen.

Yes, the current sim behavior is not wrong per se.  But like you said,
having it trap is more useful for sim's main purpose -- pre-verifying code. 


> > > The patch LGTM, but I am not a maintainer of the sim, so:
> > > 
> > > Reviewed-By: Simon Marchi <simon.marchi@efficios.com>
> > > 
> > > One note, please put the relevant information (the body of your email)
> > > into the commit log itself).
> > 
> > Yeah, sure I can expand the comment of the commit.  Updated patch/commit
> > attached.
> 
> I would give it a week for Andrew or Mike to review and approve, but
> otherwise I would feel comfortable merging it.
> 

Alright.  Last time I've checked I had write access.  I can commit & push it
myself.  Just let me know.

Best regards,
Oleg Endo

Re: [SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Simon Marchi]

On 2026-03-16 21:18, Oleg Endo wrote:
> On Wed, 2026-03-11 at 10:39 -0400, Simon Marchi wrote:
>>
>> Ok, well since we're in "undefined behavior" territory we could argue
>> that the current sim behavior is fine.  But I think the patch is still
>> good, traping is a more useful behavior to catch something you don't
>> want to happen.
> 
> Yes, the current sim behavior is not wrong per se.  But like you said,
> having it trap is more useful for sim's main purpose -- pre-verifying code. 
> 
> 
>>>> The patch LGTM, but I am not a maintainer of the sim, so:
>>>>
>>>> Reviewed-By: Simon Marchi <simon.marchi@efficios.com>
>>>>
>>>> One note, please put the relevant information (the body of your email)
>>>> into the commit log itself).
>>>
>>> Yeah, sure I can expand the comment of the commit.  Updated patch/commit
>>> attached.
>>
>> I would give it a week for Andrew or Mike to review and approve, but
>> otherwise I would feel comfortable merging it.
>>
> 
> Alright.  Last time I've checked I had write access.  I can commit & push it
> myself.  Just let me know.

Yes, please go ahead.

Simon

Re: [SH] Check FPSCR.PR for fldi0, fldi1 insns in simulator

[Oleg Endo]

On Mon, 2026-03-16 at 21:19 -0400, Simon Marchi wrote:
> 
> On 2026-03-16 21:18, Oleg Endo wrote:
> > On Wed, 2026-03-11 at 10:39 -0400, Simon Marchi wrote:
> > > 
> > > Ok, well since we're in "undefined behavior" territory we could argue
> > > that the current sim behavior is fine.  But I think the patch is still
> > > good, traping is a more useful behavior to catch something you don't
> > > want to happen.
> > 
> > Yes, the current sim behavior is not wrong per se.  But like you said,
> > having it trap is more useful for sim's main purpose -- pre-verifying code. 
> > 
> > 
> > > > > The patch LGTM, but I am not a maintainer of the sim, so:
> > > > > 
> > > > > Reviewed-By: Simon Marchi <simon.marchi@efficios.com>
> > > > > 
> > > > > One note, please put the relevant information (the body of your email)
> > > > > into the commit log itself).
> > > > 
> > > > Yeah, sure I can expand the comment of the commit.  Updated patch/commit
> > > > attached.
> > > 
> > > I would give it a week for Andrew or Mike to review and approve, but
> > > otherwise I would feel comfortable merging it.
> > > 
> > 
> > Alright.  Last time I've checked I had write access.  I can commit & push it
> > myself.  Just let me know.
> 
> Yes, please go ahead.
> 
> 

Thanks.  Committed & pushed as 17eb89e3c1cc6098f08e77d257468486a5c04ce4.

Best regards,
Oleg Endo