Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optionally move float emulation code into iram #8958

Merged
merged 14 commits into from
Nov 12, 2023

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Jul 17, 2023

following #7180 and #7188

Floating point operations are currently in IROM, they cannot be used from inside ISRs.

This PRs allows arduino builder & IDE to select where to link soft floating point code.
This was already possible for platformIO, nothing changes on this side.

From an undetermined date and until now, they have been in IROM(?) and are still there.

This is suitable for external libraries that are doing floats in ISRs.
(AccelStepper is an example)

d-a-v added 2 commits July 18, 2023 00:12
allows doing float operation in iram
suitable for libraries like AccelStepper when called from ISR
@d-a-v d-a-v added the alpha included in alpha release label Jul 18, 2023
@d-a-v
Copy link
Collaborator Author

d-a-v commented Jul 27, 2023

fpiram

tools/boards.txt.py Outdated Show resolved Hide resolved
tools/boards.txt.py Outdated Show resolved Hide resolved
tools/boards.txt.py Outdated Show resolved Hide resolved
Comment on lines 1687 to 1690
('.menu.iramfloat.no', 'Not allowed in ISR (more IRAM)'),
('.menu.iramfloat.no.build.iramfloat', '-DFP_IN_IROM'),
('.menu.iramfloat.yes', 'Allowed in ISR (less IRAM)'),
('.menu.iramfloat.yes.build.iramfloat', '-DFP_IN_IRAM'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May lose IRAM too in the 1st? Just mention in the 2nd option

: Not allowed in ISR
: Allowed in ISR (+1KiB IRAM) (or however much, we definitely know here)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, as VTABLES one. Just to signify they don't go away, just change mapping

: Not allowed in ISR (Flash)
: Allowed in ISR (IRAM)

@mcspr
Copy link
Collaborator

mcspr commented Jul 27, 2023

Also linking to #8581, but I am still as confused as when closing that. So, if FP is in IROM, what was the point of 7180 then? If is not the default, just an optional flag,

There is a weird mix of FP funcs coming from both ROM libgcc.a and from our own libgcc.a, the full list for reference https://gcc.gnu.org/onlinedocs/gccint/Soft-float-library-routines.html
Note that PIO has a special filter for .ld generation, and may skip define flags. Will fix that if we figure out the logic of fp funcs >_<

@mcspr
Copy link
Collaborator

mcspr commented Jul 28, 2023

For a quick test, trying to build with latest 3.1.2 and 2.7.4. Note that this comes from _cmpdf2.o, where #6294 update added some .o files to the .irom filter

+    *libgcc.a:_divsf3.o(.literal .text)
+    *libgcc.a:_fixsfsi.o(.literal .text)
+    *libgcc.a:_cmpdf2.o(.literal .text)
+    *libgcc.a:_cmpsf2.o(.literal .text)

Works as expected, so filters weren't somehow broken

~/d/arduino8958> nm -C .pio/build/3_1_2/firmware.elf | grep ledf2
4020a7f4 T __ledf2
~/d/arduino8958> nm -C .pio/build/2_7_4/firmware.elf | grep ledf2
40102334 T __ledf2

Otherwise unmatched stuff still goes to IRAM, where PR would also force it into IROM (!)

> nm -C .pio/build/3_1_2/firmware.elf | grep -f funcs.txt | grep 401
4010658c T __fixdfdi
401068f0 T __floatdidf
401068e4 T __floatundidf

Remaining parts of the lib are PROVIDE'd from the chip itself
(__adddf3 and below in the eagle.rom.addr.v6.ld)

4000c538 A __adddf3
4000c180 A __addsf3
4000cb94 A __divdf3
4000c8f0 A __muldf3
4000c3dc A __mulsf3
4000c688 A __subdf3
4000c268 A __subsf3

Explicitly excluded in esp-quick-toolchain post-build script, and via .ld filters

@mcspr
Copy link
Collaborator

mcspr commented Jul 28, 2023

Note that PIO has a special filter for .ld generation

diff --git a/tools/platformio-build.py b/tools/platformio-build.py
index 83b7d783..512a0cf9 100644
--- a/tools/platformio-build.py
+++ b/tools/platformio-build.py
@@ -275,17 +275,23 @@ else:
 #

 current_vtables = None
-fp_in_irom = ""
+current_fp = None
 for d in flatten_cppdefines:
     if str(d).startswith("VTABLES_IN_"):
         current_vtables = d
-    if str(d) == "FP_IN_IROM":
-        fp_in_irom = "-DFP_IN_IROM"
+    if str(d).startswith("FP_IN_"):
+        current_fp = d
+
 if not current_vtables:
     current_vtables = "VTABLES_IN_FLASH"
     env.Append(CPPDEFINES=[current_vtables])
 assert current_vtables

+if not current_fp:
+    current_fp = "FP_IN_IROM"
+    env.Append(CPPDEFINES=[current_fp])
+assert current_fp
+
 #
 # MMU
 #
@@ -363,9 +369,10 @@ app_ld = env.Command(
     join("$BUILD_DIR", "ld", "local.eagle.app.v6.common.ld"),
     join(FRAMEWORK_DIR, "tools", "sdk", "ld", "eagle.app.v6.common.ld.h"),
     env.VerboseAction(
-        "$CC -CC -E -P -D%s %s %s $SOURCE -o $TARGET"
+        "$CC -CC -E -P -D%s -D%s %s $SOURCE -o $TARGET"
         % (
             current_vtables,
+            current_fp,
             # String representation of MMU flags
             " ".join(
                 [
@@ -373,7 +380,6 @@ app_ld = env.Command(
                     for f in mmu_flags
                 ]
             ),
-            fp_in_irom,
         ),
         "Generating LD script $TARGET",
     ),

if not current_fp: branch sets default as suggested by the PR.

Repeating the note above - PR changes locations of all fp funcs to IROM, not just selected few b/c -DFP_IN_IROM becomes default (not true right now, various fp funcs can be either in IROM or IRAM)

@d-a-v
Copy link
Collaborator Author

d-a-v commented Nov 7, 2023

If default was float in iram until v2.7, should we revert this default 3.x change back to as it was ?
I don't remember that it was done on purpose.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Nov 12, 2023

Here is the new menu proposal: default is now in iram, text simplified per review
iramfloat

edit order is now reversed, floats in flash is the default

@mcspr
Copy link
Collaborator

mcspr commented Nov 12, 2023

If default was float in iram until v2.7, should we revert this default 3.x change back to as it was ?
I don't remember that it was done on purpose.

Tasmota does not seem to use 3.x.x (if ever used it?), so direct comparison does not really work with their builds. .ld change was true for 2.7.4, but 3.x moved even more into IROM. With your changes we move conversions into IROM by default (to, from float / double to signed / unsigned pairs. see gcc link for reference)

I also tried ESPeasy beta builds - IROM opt builds and saves 552 bytes. IRAM does not, too much in there already to fit extra funcs. I wonder if conversions actually break something in some sensor code somewhere that accidentally converts to or from float though

@d-a-v d-a-v merged commit d0f7293 into esp8266:master Nov 12, 2023
28 checks passed
@d-a-v d-a-v deleted the float-in-iram branch November 12, 2023 22:53
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* optionally move float emulation code into iram
allows doing float operation in iram
suitable for libraries like AccelStepper when called from ISR
* proposed changes for pio from @mcspr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha included in alpha release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants