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

64-bit NaN-boxing support #1655

Closed
wants to merge 13 commits into from
Closed

Conversation

dpgeorge
Copy link
Member

This is now ready for review and merging. It's pretty much the same as #1593, but now I split the conceptual changes into different commits. There are basically 4 types of changes:

  1. Changing types from mp_uint_t/mp_obj_t to size_t/uintptr_t to allow sizeof(mp_obj_t) != sizeof(void*).
  2. Adding MP_ROM_* macros for creating rom objects. This is a single commit (and slightly different from how it was done in Preliminary but working implementation of 64-bit NaN boxing for 32-bit archs #1593).
  3. Wrapping all obj-ptr access in MP_OBJ_CAST/MP_OBJ_UNCAST. This is a single commit.
  4. Adding OBJ_REPR_D (nan boxing). This is a single commit.

It's probably best to look at the individual commits, rather than the whole thing at once.

The last outstanding thing: I think it would be much easier to understand the code if the macros are renamed: MP_OBJ_CAST becomes MP_OBJ_TO_PTR, and MP_OBJ_UNCAST becomes MP_OBJ_FROM_PTR. Unless there are better ideas.

@stinos
Copy link
Contributor

stinos commented Nov 27, 2015

In windows' mpconfigport there's a #define BYTES_PER_WORD sizeof(mp_int_t); if a 32bit build defines mp_int_t as 64bit int that becomes 8, is that ok or should it still be 4? Maybe the name should be changed cause intuitively I'd say BYTES_PER_WORD is 4 for 32bit builds (ok there are multiple definitions of 'word' possible, but that just adds to the confusion) but looking at the places where it's used that might not always be the correct assumption?

Anyway when applying the change as per the last commit message I get

warning C4047: 'function' : 'mp_obj_t' differs in levels of indirection from 'void *'
warning C4024: 'mp_obj_is_exception_instance' : different types for formal and actual parameter 1

anywhere nlr_raise is used (MICROPY_NLR_SETJMP=1 for msvc build).

@dhylands
Copy link
Contributor

Thinking out loud: does this mean that we could add a "compactable" pointer type which perhaps needs a double dereference? It's obviously not as simple as that, but seems like it might be a first step.

So if I'm reading this correctly, MICROPY_OBJ_REPR_C could be used on 32-bit machines with a penalty in that the size of an mp_obj_t just increased. And with the appropriate sw floating point support, you could then use 64-bit floats.

@pfalcon
Copy link
Contributor

pfalcon commented Nov 27, 2015

Thinking out loud: does this mean that we could add a "compactable" pointer type which perhaps needs a double dereference?

The reason I added MP_OBJ_CAST/MP_OBJ_UNCAST is because I did initial steps of experimenting with compaction and other advanced GC types.

This allows the mp_obj_t type to be configured to something other than a
pointer-sized primitive type.

This patch also includes additional changes to allow the code to compile
when sizeof(mp_uint_t) != sizeof(void*), such as using size_t instead of
mp_uint_t, and various casts.
To use, put the following in mpconfigport.h:

    #define MICROPY_OBJ_REPR (MICROPY_OBJ_REPR_D)
    #define MICROPY_FLOAT_IMPL (MICROPY_FLOAT_IMPL_DOUBLE)
    typedef int64_t mp_int_t;
    typedef uint64_t mp_uint_t;
    #define UINT_FMT "%llu"
    #define INT_FMT "%lld"

Currently does not work with native emitter enabled.
@dpgeorge
Copy link
Member Author

Thanks guys, PR updated based on your comments:

  • printf formatting codes fixed
  • windows build should not error anymore with nlr_raise; @stinos can you please check?
  • changed MP_OBJ_CAST to MP_OBJ_TO_PTR, and MP_OBJ_UNCAST to MP_OBJ_FROM_PTR (I prefer this to OBJ2PTR, PTR2OBJ).

In windows' mpconfigport there's a #define BYTES_PER_WORD sizeof(mp_int_t); if a 32bit build defines mp_int_t as 64bit int that becomes 8, is that ok or should it still be 4? Maybe the name should be changed cause intuitively I'd say BYTES_PER_WORD is 4 for 32bit builds

Yes, BYTES_PER_WORD is now confusing. In the code it seems to be used exclusively for mp_uint_t and mp_int_t sized things, so should really be BYTES_PER_OBJECT_WORD (or, should just use sizeof(mp_uint_t) in the code itself, this is clearer I think). We can fix this later.

So if I'm reading this correctly, MICROPY_OBJ_REPR_D could be used on 32-bit machines with a penalty in that the size of an mp_obj_t just increased. And with the appropriate sw floating point support, you could then use 64-bit floats.

Correct, and it wouldn't use the heap for fp objects. You could also just use double precision heap objects at the moment by #define MICROPY_FLOAT_IMPL (MICROPY_FLOAT_IMPL_DOUBLE).

@pfalcon
Copy link
Contributor

pfalcon commented Nov 29, 2015

Changing types from mp_uint_t/mp_obj_t to size_t/uintptr_t to allow sizeof(mp_obj_t) != sizeof(void*).

I predict people (including me) doing mistakes with that for a long time. It would with having "now set of rules to use for types is: ...", to be able to refer to.

@stinos
Copy link
Contributor

stinos commented Nov 29, 2015

nlr_raise is fixed, thanks. I ran the tests now and there are some failures but I guess you know about these? Range printing is broken because mp_vprintf does not handle %lld and the float2int 'power of 2'/'power of 10' tests fail, I assume because they make assumptions about float representation.

@pfalcon
Copy link
Contributor

pfalcon commented Nov 29, 2015

And btw, I'm +10 for at least parts of this being merged ASAP, e.g. MP_OBJ_TO_PTR/OBJ_FROM_PTR part. And I thought that this complete patch would be good enough test of MP_OBJ_TO_PTR/OBJ_FROM_PTR, but it apparently won't, as pointers are unaffected by this implementation. So, would be nice to have ability to test them, e.g. by offsetting values back and forth. (But that's a separate feature, which shouldn't block merge of MP_OBJ_TO_PTR/OBJ_FROM_PTR commit itself).

@dpgeorge
Copy link
Member Author

I ran the tests now and there are some failures but I guess you know about these?

Yes, there are a few that need fixing, but it can be done later.

And btw, I'm +10 for at least parts of this being merged ASAP

Yes, I think it's good to merge all of it now, since it doesn't break anything anymore (with OBJ_RERP_A).

So, would be nice to have ability to test them, e.g. by offsetting values back and forth.

That's not possible to do in a simple way because pointers pointing to ROM can't be linked with an offset (but MP_ROM_PTR is there exactly so you can play with different ways of storing ROM data vs RAM data).

@dpgeorge
Copy link
Member Author

Merged! See b8cfb0d for last commit.

I predict people (including me) doing mistakes with [void* vs mp_obj_t etc] for a long time. It would with having "now set of rules to use for types is: ...", to be able to refer to.

Yes. I think a 64-bit nan boxing build should be part of Travis. I would like it to be stmhal built with a 64-bit boxing option, but for that we need to get software double precision working (shouldn't be too hard) and then double precision math functions (a lot more work).

@dpgeorge dpgeorge closed this Nov 29, 2015
@dpgeorge dpgeorge deleted the nan-boxing-v2 branch November 29, 2015 14:31
@pfalcon
Copy link
Contributor

pfalcon commented Nov 29, 2015

That's not possible to do in a simple way because pointers pointing to ROM can't be linked with an offset (but MP_ROM_PTR is there exactly so you can play with different ways of storing ROM data vs RAM data).

I finally grasped why. Well, then MP_OBJ_FROM_PTR should be (for test we discuss) a macro/inline function which passes-thru address belonging to .text, and offsetting anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants