Allow boards to enable the `micropython.native` decorator #2271

theacodes · 2019-11-05T22:32:31Z

Adds CIRCUITPY_ENABLE_MPY_NATIVE for mpconfigboard.mk that allows boards to enable the micropython.native decorator.

I am 100% open to suggestions on this PR. I wanted to open a PR that more or less does the minimal amount of work to enable this. As such, I had to disable a few warnings to get this to build. The upstream code for this has deviated a bit, so if it's preferable to backport that code instead I'm up for it.

Related to #1248. I would enable it and test and mark as closes, but I don't have an nRF board to test with. It works fine for my SAMD51 boards (Feather M4 Express & Winterbloom Sol).

Adds the `CIRCUITPY_ENABLE_MPY_NATIVE` for `mpconfigboard.mk` that enables the `micropython.native` decorator.

dhalbert · 2019-11-05T23:47:08Z

I think this is laudable, because it's a port-agnostic way of getting speedups (at the expense of RAM). At the same time, there is further work that could be done to speed up CircuitPython in general, by probably about a factor of 2: see #2142.

I do worry about fixes to MicroPython related to this that haven't been merged. It's been a long time since we merged from upstream, so I think it would be worth looking at that. You could do a trial merge into a junk branch and see what's affected, or just browse the MicroPython "blame" info for the relevant code sections.

theacodes · 2019-11-06T00:04:32Z

Yeah, I'm happy to take the time and do this as "correctly" as possible and see if the latest code can be pulled in. I just didn't know if we've done any sort of piecemeal merging from upstream before.

#2142 is great because it improves the speed of all ports without any need to opt-in (unlike this). This is a great parallel effort, though.

ladyada · 2019-11-06T00:14:47Z

(psst, im into this too, there's a lot of times we could have some optimized code for drivers!)

theacodes · 2019-11-06T00:37:01Z

After some initial exploration it looks like the changes to emitnative.c and friends are pretty extensive. I could try to merge these in, but it seems like it might be likely to also effect the bytecode stuff as well (several of the changes involved touch the bytecode stuff).

Not sure how we feel about it right now. We have three options:

Merge this as-is for now, leave it as "experimental". Update this code along with the rest of the micropython code during the next merge from upstream.
Don't merge this yet. Wait until the next merge from upstream to try to enable this.
Attempt to merge just the emitnative.c changes (and friends) without affecting bytecode compatibility.

dhalbert · 2019-11-06T00:43:52Z

I'm happy to have us try this, since it's all optional anyway. Yes, there may be bugs, but that's true of anything. We have no short-term plans for a merge from upstream: certainly not for 5.0.0

dhalbert

Just one question: it looks great!

dhalbert · 2019-11-05T23:42:17Z

py/objfun.c

@@ -546,7 +546,9 @@ STATIC mp_uint_t convert_obj_for_inline_asm(mp_obj_t obj) {
 STATIC mp_obj_t fun_asm_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const mp_obj_t *args) {
    mp_obj_fun_asm_t *self = self_in;

+    #pragma GCC diagnostic ignored "-Wint-conversion"


Have you tracked down why you need to do this? Is this older code that could use a change in parameter types, or something like that?

Fixed in 3439c36, this one turned out to be pretty easy to fix once I looked at what the code was trying to do.

theacodes · 2019-11-06T01:28:30Z

Fwiw I spent about an hour trying option (3) and got reasonably far, but there's some significant changes in compile.c that I'm not confident enough to try to graft on to our fork. Maybe down the road.

theacodes · 2019-11-06T01:53:04Z

In case anyone is curious about my work to update all of the native emitters there's a branch here. Perhaps after supercon I'll have some time to get that into a mergeable state.

dhalbert · 2019-11-07T17:03:53Z

@theacodes Are you satisfied with the PR now, or are you still exploring? Happy to merge at this point.

theacodes · 2019-11-07T17:33:18Z

Satisfied for now. I did some more exploration and I think it'd be easier to just do a full merge than graft the newer compiler code into our tree. This is good to go. Btw, I do actually have an nRF board so once I find some free time I'll test it out on nRF boards, but this should be good to go for SAMD51 boards.

…

On Thu, Nov 7, 2019, 9:04 AM Dan Halbert ***@***.***> wrote: @theacodes <https://github.com/theacodes> So are you satisfied with the PR now, or are you still exploring? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2271?email_source=notifications&email_token=AAB5I46LQXM47DL54NTOKXLQSRDABA5CNFSM4JJLOEF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDNDKWY#issuecomment-551171419>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I4ZCHQD6NXH7FDXR4T3QSRDABANCNFSM4JJLOEFQ> .

dhalbert · 2019-11-07T17:59:55Z

Sounds good. I'll wait for you to test on nRF and then merge if that goes fine.

theacodes · 2019-11-08T03:07:56Z

Tested on nRF (Circuit Playground Bluefruit alpha)

Build size without CIRCUITPY_ENABLE_MPY_NATIVE:

766616 bytes free in flash out of 1048576 bytes ( 1024.0 kb ).
232344 bytes free in ram for stack out of 245760 bytes ( 240.0 kb ).

Build size with it:

766596 bytes free in flash out of 1048576 bytes ( 1024.0 kb ).
232344 bytes free in ram for stack out of 245760 bytes ( 240.0 kb ).

So 20 more bytes?

Test code:

import time
import micropython


def math_py():
    x = 5
    for n in range(100000):
        x += 1
        x -= 1
        x *= 10
        x /= 10
        

@micropython.native
def math_native():
    x = 5
    for n in range(100000):
        x += 1
        x -= 1
        x *= 10
        x /= 10
        

now = time.monotonic()
math_py()
print("Python elapsed: ", time.monotonic() - now)

now = time.monotonic()
math_native()
print("Native elapsed: ", time.monotonic() - now)

Result:

Python elapsed:  3.592
Native elapsed:  2.125

🎉

dhalbert

OK, great! On to real-world testing and use!

gpshead · 2019-11-08T19:36:46Z

Thanks for this! I've been manually tweaking my repos when I wanted these for the past year or two.

Oddities if you're used to CPython: @micropython.asm_thumb et. al. aren't actual callable decorators. On builds without these enabled, the file fails to parse. Minor bummer, but that's micro-life. Not a big deal. What I did in this scenario is just keep different implementations in different files, only loading one of them:

try:
    # Optimized version - requires a custom CircuitPython build.
    from asm_thumb import fractal
    from asm_thumb import monobitmap
    HAVE_ASM = True
except (ImportError, SyntaxError):
    import fractal
    import monobitmap
    HAVE_ASM = False

As seen in https://github.com/gpshead/epaper-circuitpython/blob/master/main.py

Other things... mpycross doesn't handle @micropython.asm_thumb files (I haven't tried with native or viper). Meaning a lot more ram is required just to load code using it. It seems plausible to implement but I'm not sure if it ever was. I haven't checked upstream micropython or tried to dive into that rabbit hole.

theacodes · 2019-11-08T20:17:53Z

I think upstream implemented it but we'd have to do a full merge to get that feature. I definitely agree that supporting both boards with and without this feature is a bit tedious.

…

On Fri, Nov 8, 2019 at 11:36 AM Gregory P. Smith ***@***.***> wrote: Thanks for this! I've been manually tweaking my repos when I wanted these for the past year or two. Oddities if you're used to CPython: @micropython.asm_thumb et. al. aren't *actual* callable decorators. On builds without these enabled, the file fails to parse. Minor bummer, but that's micro-life. Not a big deal. What I did in this scenario is just keep different implementations in different files, only loading one of them: try: # Optimized version - requires a custom CircuitPython build. from asm_thumb import fractal from asm_thumb import monobitmap HAVE_ASM = True except (ImportError, SyntaxError): import fractal import monobitmap HAVE_ASM = False *As seen in https://github.com/gpshead/epaper-circuitpython/blob/master/main.py <https://github.com/gpshead/epaper-circuitpython/blob/master/main.py>* Other things... mpycross doesn't handle @micropython.asm_thumb files (I haven't tried with native or viper). Meaning a lot more ram is required just to load code using it. It seems plausible to implement but I'm not sure if it ever was. I haven't checked upstream micropython or tried to dive into that rabbit hole. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2271?email_source=notifications&email_token=AAB5I47MMYTVOT3FRP7P633QSW5VLA5CNFSM4JJLOEF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDTEODY#issuecomment-551962383>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I45XWOHKMHITNUNTZSTQSW5VLANCNFSM4JJLOEFQ> .

tannewt · 2019-11-10T17:13:36Z

@gpshead We should add noop decorators to the micropython module in Blinka: https://github.com/adafruit/Adafruit_Blinka/blob/master/src/micropython.py

rdagger · 2020-05-11T16:50:39Z

I tried the sample code above on an nRF52840 Feather running CircuitPython 5.3.0-184-g90bd93180 on 2020-05-09. The native runs slightly slower:

Python elapsed: 4.591
Native elapsed: 4.59601

Python elapsed: 4.59094
Native elapsed: 4.59497

Python elapsed: 4.59009
Native elapsed: 4.59399

Allow boards to enable the micropython.native decorator

c7195c4

Adds the `CIRCUITPY_ENABLE_MPY_NATIVE` for `mpconfigboard.mk` that enables the `micropython.native` decorator.

dhalbert reviewed Nov 6, 2019

View reviewed changes

Fix bad call to mp_arg_check_num

3439c36

dhalbert approved these changes Nov 8, 2019

View reviewed changes

dhalbert merged commit 9115667 into adafruit:master Nov 8, 2019

theacodes mentioned this pull request Nov 8, 2019

nRF Port: Enable @micropython.native, @micropython.viper and @micropython.asm_thumb #1248

Closed

gpshead mentioned this pull request Nov 8, 2019

Enable float instructions support for asm_thumb #2276

Closed

theacodes mentioned this pull request Nov 26, 2019

Add stubs for micropython.natve, viper, and asm_thumb adafruit/Adafruit_Blinka#183

Merged

rgov mentioned this pull request Feb 11, 2024

Support @micropython.native on RP2040 #8902

Open

Allow boards to enable the micropython.native decorator #2271

Allow boards to enable the micropython.native decorator #2271

Uh oh!

Conversation

theacodes commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhalbert commented Nov 5, 2019

Uh oh!

theacodes commented Nov 6, 2019

Uh oh!

ladyada commented Nov 6, 2019

Uh oh!

theacodes commented Nov 6, 2019

Uh oh!

dhalbert commented Nov 6, 2019

Uh oh!

dhalbert left a comment

Choose a reason for hiding this comment

Uh oh!

dhalbert Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

theacodes Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

theacodes commented Nov 6, 2019

Uh oh!

theacodes commented Nov 6, 2019

Uh oh!

dhalbert commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theacodes commented Nov 7, 2019 via email

Uh oh!

dhalbert commented Nov 7, 2019

Uh oh!

theacodes commented Nov 8, 2019

Uh oh!

dhalbert left a comment

Choose a reason for hiding this comment

Uh oh!

gpshead commented Nov 8, 2019

Uh oh!

theacodes commented Nov 8, 2019 via email

Uh oh!

tannewt commented Nov 10, 2019

Uh oh!

rdagger commented May 11, 2020

Uh oh!

Uh oh!

Allow boards to enable the `micropython.native` decorator #2271

Allow boards to enable the `micropython.native` decorator #2271

theacodes commented Nov 5, 2019 •

edited

Loading

dhalbert commented Nov 7, 2019 •

edited

Loading