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

Support for arm64 / Apple M1 #33

Closed
grantpassmore opened this issue Dec 6, 2021 · 11 comments
Closed

Support for arm64 / Apple M1 #33

grantpassmore opened this issue Dec 6, 2021 · 11 comments

Comments

@grantpassmore
Copy link

Thank you for the incredibly useful landmarks library! It works flawlessly for us on x86, but we seem to be out of luck for now on our Apple M1s. Is there a nontrivial technical obstruction for supporting arm64? If there is anything we could do to help, we would be happy to try.

@nojb
Copy link
Member

nojb commented Dec 6, 2021

I am guessing this is due to the need for a specific implementation of the caml_highres_clock primitive in https://github.com/LexiFi/landmarks/blob/master/src/clock.c which uses the rdtsc instruction on x86. I'm not very familiar with arm64, but I imagine that there should be some equivalent instruction. cc @mlasson

@grantpassmore
Copy link
Author

grantpassmore commented Dec 6, 2021

Thank you for investigating! In case it is useful, my (very much non-expert) understanding is that the equivalent to rdtsc can be obtained on arm64 via the cntvct_el0 register, e.g., something like

asm volatile ("mrs %0, cntvct_el0" : "=r" (_var_))

cf. https://community.arm.com/support-forums/f/architectures-and-processors-forum/7019/arm-v8-pmu-cycle-counter .

@nojb
Copy link
Member

nojb commented Dec 6, 2021

We don't have any arm64 hardware to test this on, but you can give it a try by adding this to clock.c:

#elif defined(__aarch64__)
    uint64_t v;
    asm volatile ("mrs %0, cntvct_el0" : "=r"(v));
    return caml_copy_int64(v);
#else
    ...

@grantpassmore
Copy link
Author

Fantastic, thank you. We will test today and report our findings!

@c-cube
Copy link

c-cube commented Dec 6, 2021

Is that the same primitive that mtime exposes?

@mlasson
Copy link
Member

mlasson commented Dec 6, 2021

@grantpassmore

Fantastic, thank you. We will test today and report our findings!

Do not hesitate to file a PR if you want afterward !

@c-cube

Is that the same primitive that mtime exposes?
No, but actually any monotonic wall-clock time.
Maybe we probably should add a way to support other clocks.

@c-cube
Copy link

c-cube commented Dec 6, 2021

maybe a dune variant to provide the clock implementation could help? One could provide a mtime-based variant, for exotic architectures where it's better supported (which might be no better than landmarks' current coverage).

@k4rtik
Copy link

k4rtik commented Nov 17, 2022

Is there anyone still looking at this issue? cc: @grantpassmore

@johnyob
Copy link
Contributor

johnyob commented Feb 8, 2023

We've experimented with this fix on: https://github.com/johnyob/landmarks/tree/arm-support.

It has been tested with Apple M1 macs.

@nojb
Copy link
Member

nojb commented Feb 9, 2023

We've experimented with this fix on: https://github.com/johnyob/landmarks/tree/arm-support.

It has been tested with Apple M1 macs.

The patch looks fine, can you open a PR? Thanks!

@nojb
Copy link
Member

nojb commented Feb 11, 2023

Fixed in #35

@nojb nojb closed this as completed Feb 11, 2023
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

No branches or pull requests

6 participants