Skip to content

Introduce atomic loads in Cmm and Mach IRs#10943

Merged
gasche merged 1 commit intoocaml:trunkfrom
OlivierNicole:fix_atomic_load
Feb 3, 2022
Merged

Introduce atomic loads in Cmm and Mach IRs#10943
gasche merged 1 commit intoocaml:trunkfrom
OlivierNicole:fix_atomic_load

Conversation

@OlivierNicole
Copy link
Copy Markdown
Contributor

@OlivierNicole OlivierNicole commented Jan 24, 2022

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.

@OlivierNicole OlivierNicole mentioned this pull request Jan 24, 2022
Closed
kayceesrk
kayceesrk reviewed Jan 25, 2022
View reviewed changes
@kayceesrk
Copy link
Copy Markdown
Contributor

kayceesrk commented Jan 25, 2022

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.

ocaml/asmcomp/mach.mli

Line 55 in 488a28a

| 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 mentioned this pull request Jan 25, 2022
Closed
@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
Copy Markdown
Contributor Author

OlivierNicole commented Jan 27, 2022

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
Copy Markdown
Contributor

kayceesrk commented Jan 27, 2022

The changes look good to me.

@kayceesrk kayceesrk mentioned this pull request Feb 1, 2022
Merged
kayceesrk
kayceesrk reviewed Feb 1, 2022
View reviewed changes
kayceesrk
kayceesrk reviewed Feb 1, 2022
View reviewed changes
kayceesrk
kayceesrk approved these changes Feb 1, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

OlivierNicole commented Feb 1, 2022

I integrated your comments.

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

gasche commented Feb 3, 2022

Thanks! This looks good, so I merged.

@dra27 dra27 mentioned this pull request Apr 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xavierleroy xavierleroy xavierleroy left review comments

@kayceesrk kayceesrk kayceesrk approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@OlivierNicole @kayceesrk @gasche @xavierleroy