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

Introduce atomic loads in Cmm and Mach IRs #10943

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

OlivierNicole
Copy link
Contributor

The Patomic_load primitive gives lieu to a non-atomic load (i.e. Cload { is_atomic = false; _ }) in Cmm. This is correct in x86 but needs to be lowered down the pipeline to ease support of other architectures.

In addition, this PR proposes to add an ~is_atomic argument to the mk_load_mut helper to make the atomicity of loads explicit during Cmm generation.

asmcomp/cmm_helpers.mli Outdated Show resolved Hide resolved
@kayceesrk
Copy link
Contributor

Note that even with this change, the Cmm to Mach translation ignores the atomicity.

ocaml/asmcomp/selectgen.ml

Lines 449 to 451 in 488a28a

| (Cload {memory_chunk; mutability}, [arg]) ->
let (addr, eloc) = self#select_addressing memory_chunk arg in
(Iload(memory_chunk, addr, mutability), [eloc])

Further work is needed to incorporate the atomicity argument into the definition of Iload.

| Iload of Cmm.memory_chunk * Arch.addressing_mode * Asttypes.mutable_flag

It is useful to incorporate this change into this PR. Best to change Iload into an inline record for readability. Perhaps @gasche may have other thoughts before I ask you to do more work.


On a related note, the atomic store, fetch-and-add and compare-and-swap are also converted early into an external calls in cmmgen. This may also need to be lowered for efficient codegen.

ocaml/asmcomp/cmmgen.ml

Lines 1056 to 1061 in 488a28a

| Patomic_exchange ->
Cop (Cextcall ("caml_atomic_exchange", typ_val, [], false),
[transl env arg1; transl env arg2], dbg)
| Patomic_fetch_add ->
Cop (Cextcall ("caml_atomic_fetch_add", typ_int, [], false),
[transl env arg1; transl env arg2], dbg)

ocaml/asmcomp/cmmgen.ml

Lines 1115 to 1117 in 488a28a

| Patomic_cas ->
Cop (Cextcall ("caml_atomic_cas", typ_int, [], false),
[transl env arg1; transl env arg2; transl env arg3], dbg)

@OlivierNicole OlivierNicole force-pushed the fix_atomic_load branch 2 times, most recently from 937bce2 to 1e21aaf Compare January 27, 2022 16:32
@OlivierNicole OlivierNicole changed the title Translate Patomic_load to atomic load in Cmm Introduce atomic loads in Cmm and Mach IRs Jan 27, 2022
@OlivierNicole
Copy link
Contributor Author

It is useful to incorporate this change into this PR. Best to change Iload into an inline record for readability.

This is the case in the new version.

@kayceesrk
Copy link
Contributor

The changes look good to me.

asmcomp/cmm_helpers.mli Outdated Show resolved Hide resolved
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.

I've left a few comments that clarify the API.

@OlivierNicole
Copy link
Contributor Author

I integrated your comments.

@gasche gasche merged commit ebb9f0d into ocaml:trunk Feb 3, 2022
@gasche
Copy link
Member

gasche commented Feb 3, 2022

Thanks! This looks good, so I merged.

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