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

[refactor] simplify the definition of atomic functions #13397

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

gasche
Copy link
Member

@gasche gasche commented Aug 24, 2024

In trunk, all atomic functions exposed in the runtime are also exposed as language primitives in our intermediate representations (lambda, clambda). But except for Patomic_load, which benefits from dedicated code generation, they are all transformed into C calls on all backends.

The present PR simplifies the code noticeably by removing the intermediate primitives, by producing C calls directly in lambda/translprim.ml.

This reduces the amount of boilerplate to modify to implement atomic record fields (ocaml/RFCs#39).

In trunk, all atomic functions exposed in the runtime are also exposed
as language primitives in our intermediate representations (lambda,
clambda). But except for `Patomic_load`, which benefits from
dedicated code generation, they are all transformed into C calls
on all backends.

The present PR simplifies the code noticeably by removing the
intermediate primitives, by producing C calls directly in
lambda/translprim.ml.

This reduces the amount of boilerplate to modify to implement
atomic record fields (ocaml/RFCs#39).

Co-authored-by: Clément Allain <clef-men@orange.fr>
Copy link
Contributor

@kayceesrk kayceesrk left a comment

Choose a reason for hiding this comment

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

LGTM.

@OlivierNicole
Copy link
Contributor

But except for Patomic_load, which benefits from dedicated code generation, they are all transformed into C calls on all backends.

I am wondering whether we should make more atomic operations like Patomic_load. I just opened #13403 to track that discussion, not for the sake of being contrarian, but because I’ve been putting it off for a while and this PR prompted me to express it.

@kayceesrk
Copy link
Contributor

My feeling is that we should go ahead with this PR to simplify the implementation of ocaml/RFCs#39. The code being removed here is mostly boilerplate and can be added back if certain operations in Atomic module need to be inlined.

@OlivierNicole OlivierNicole added the run-thread-sanitizer This label makes the CI run the testsuite with TSAN enabled label Aug 27, 2024
@OlivierNicole
Copy link
Contributor

I agree that it is probably best not to block this simplification PR and discuss #13403 independently.

@lthls
Copy link
Contributor

lthls commented Aug 27, 2024

If you're using C calls immediately, is there any reason to keep the definition as %-primitives ? I would change atomic.ml to use the C primitives directly, that would save you the trouble of defining them manually in Translprim.

@gasche
Copy link
Member Author

gasche commented Aug 27, 2024

@OlivierNicole one obvious downside of "compiling to one or two instructions" is that we now have to maintain one compilation scheme for each of those operations, for each backend. We can arguably just copy whatever gcc/clang are doing, but this is extra work and maintenance. Is there a clear performance case for doing this extra work?

@lthls I considered doing this. But having the %foo primitive around gives us slightly more freedom to change the implementation strategy without touching the stdlib (or possibly, third-party code): we can change back and forth between primitives and C calls without changing OCaml code at all. Given the very small maintenance cost of the %foo blurb, I think this is a good compromise.

@gasche
Copy link
Member Author

gasche commented Aug 27, 2024

Is there a clear performance case for doing this extra work?

More precisely, my suggestion is to try to find a program whose bottleneck are atomic operations, implement backend support for just one backend for the atomic you care about, and see if there is a measurable benefit.

@gasche
Copy link
Member Author

gasche commented Aug 27, 2024

Thanks everyone! I fully agree with @kayceesrk' assessment :-) and propose to merge right away.

@gasche gasche merged commit 6475f63 into ocaml:trunk Aug 27, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-entry-needed run-thread-sanitizer This label makes the CI run the testsuite with TSAN enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants