Sitelet https://github.com/micropython/micropython/pull/15812
Skip to content

tools/mpy_ld.py: Support jumping more than 2k on armv6m architectures. - #15812

Merged
dpgeorge merged 1 commit into
micropython:masterfrom
dpgeorge:tools-mpy-ld-armv6m-long-jump
Sep 19, 2024
Merged

dpgeorge merged 1 commit into
micropython:masterfrom
dpgeorge:tools-mpy-ld-armv6m-long-jump

Conversation

@dpgeorge

@dpgeorge dpgeorge commented Sep 9, 2024

Copy link
Copy Markdown
Member

Summary

Native .mpy files targetting armv6m (eg RP2040) cannot currently have more than about 2kiB of native code (between the start of the file and the init function).

This PR fixes that by using bigger jumps to jump to the init function.

Testing

Tested with examples/natmod/features0/ by forcing mpy_ld.py to always emit the long jump. It works.

@dpgeorge dpgeorge added the tools Relates to tools/ directory in source, or other tooling label Sep 9, 2024
@dpgeorge

dpgeorge commented Sep 9, 2024

Copy link
Copy Markdown
Member Author

This is an alternative to #12241 that uses less bytes (8) to encode the jump.

@codecov

codecov Bot commented Sep 9, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.44%. Comparing base (46d8db8) to head (8b35f2c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #15812   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files         163      163           
  Lines       21296    21296           
=======================================
  Hits        20964    20964           
  Misses        332      332           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions

github-actions Bot commented Sep 9, 2024

Copy link
Copy Markdown

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@jonnor

jonnor commented Sep 9, 2024

Copy link
Copy Markdown
Contributor

This looks nice and simple. I will test it later. Probably the -2 adjustment of the offset is what I missed when I tried to implement this.

This function must return the same number of bytes for the encoding of the jump regardless of the value of entry.

As it stands, the function does not obey this property - it's either 2 or 4 bytes. As you mentioned in the review of my code, this can cause issues because the length of instructions is used else where to compute the layout. Is the plan to skip the conditional and always do the long jump (once verified)?

@dpgeorge dpgeorge added this to the release-1.24.0 milestone Sep 10, 2024
@dpgeorge

Copy link
Copy Markdown
Member Author

As it stands, the function does not obey this property - it's either 2 or 4 bytes

No, it always emits 8 bytes. See the struct.pack("<HHHH", ...) at the end of the function. There is no other return in the function.

@vshymanskyy

vshymanskyy commented Sep 13, 2024 •

Copy link
Copy Markdown
Contributor

I have run this change with wasm2mpy tests (with and without forcing long jumps) and noticed no regressions.
Coremark builds fine, but still doesn't run properly due to unknown reason.

@jonnor

jonnor commented Sep 14, 2024

Copy link
Copy Markdown
Contributor

As it stands, the function does not obey this property - it's either 2 or 4 bytes

No, it always emits 8 bytes. See the struct.pack("<HHHH", ...) at the end of the function. There is no other return in the function.

You are right, of course. I do not know where my head was at.

@jonnor

jonnor commented Sep 14, 2024

Copy link
Copy Markdown
Contributor

Tested with a module that exceeds the 2k limitation (before this commit, fails with AssertionError: 3295 in assert b_off >> 11 == 0 or b_off >> 11 == -1, b_off). Builds successfully and looks to run ok on a RPI Pico. So I think this is good to go @dpgeorge !

@dpgeorge

Copy link
Copy Markdown
Member Author

Thanks @jonnor and @vshymanskyy for testing and review!

Native .mpy files targetting armv6m (eg RP2040) cannot currently have more
than about 2kiB of native code (between the start of the file and the init
function).

This commit fixes that by using bigger jumps to jump to the init function.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
dpgeorge force-pushed the tools-mpy-ld-armv6m-long-jump branch from 16482fe to 8b35f2c Compare September 19, 2024 04:53
@dpgeorge
dpgeorge merged commit 8b35f2c into micropython:master Sep 19, 2024
@dpgeorge
dpgeorge deleted the tools-mpy-ld-armv6m-long-jump branch September 19, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools Relates to tools/ directory in source, or other tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants