Skip to content

Conversation

shravanrn
Copy link
Collaborator

@shravanrn shravanrn commented Oct 2, 2023

Implement wasm2c atomic ops and shared memory ops with C11 atomics as discussed in #2268 (comment)

This is the first of several PRs to fully support the various primitives in the threads/shared_memory/atomics proposal.

@shravanrn shravanrn requested review from keithw and sbc100 October 2, 2023 01:58
@shravanrn shravanrn force-pushed the atomic_sharedmem2 branch 3 times, most recently from c8b4b83 to cd20126 Compare October 3, 2023 18:58
@shravanrn
Copy link
Collaborator Author

@keithw @sbc100 any chance you could have a look at this? This is blocking for the next patch for wait/notify

@keithw
Copy link
Member

keithw commented Oct 12, 2023

Sure, can we finish tail calls and then land this?

@shravanrn
Copy link
Collaborator Author

shravanrn commented Oct 12, 2023

Sure, can we finish tail calls and then land this?

@keithw Sure, let's kick off the the review process for now and we can coordinate before landing?

Copy link
Member

@keithw keithw left a comment

Choose a reason for hiding this comment

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

I'm definitely no shared-memory expert, but this looks pretty reasonable % comments.

@shravanrn shravanrn force-pushed the atomic_sharedmem2 branch 2 times, most recently from 1d21bed to 1f35f1e Compare October 18, 2023 23:37
@shravanrn shravanrn marked this pull request as draft November 15, 2023 03:56
@shravanrn shravanrn force-pushed the atomic_sharedmem2 branch 2 times, most recently from 1c1e812 to 97be058 Compare December 13, 2023 22:49
@shravanrn shravanrn marked this pull request as ready for review December 13, 2023 22:50
@shravanrn shravanrn marked this pull request as draft December 14, 2023 00:59
@shravanrn shravanrn force-pushed the atomic_sharedmem2 branch 8 times, most recently from d829019 to 2c9fb8b Compare December 16, 2023 02:31
@shravanrn shravanrn marked this pull request as ready for review December 16, 2023 02:50
@shravanrn
Copy link
Collaborator Author

@keithw @sbc100 Since we are waiting for the discussion on the other PRs, can we try landing this? If this looks good, could one of you sign off on this?

@sbc100
Copy link
Member

sbc100 commented Dec 16, 2023

Sorry I didn't get to looking at this yet. I will try to take a look in the morning (PST)

@shravanrn
Copy link
Collaborator Author

@keithw @sbc100 Happy new year all! :) Just a ping to have a look when you get the chance

@shravanrn
Copy link
Collaborator Author

@keithw @sbc100 Happy new year all! :) Just a ping to have a look when you get the chance

@keithw @sbc100 Bumping this up

@sbc100
Copy link
Member

sbc100 commented Jan 9, 2024

Apologies for taking so long to look at this one. Will try to get to it soon/today.

@shravanrn
Copy link
Collaborator Author

@keithw Just bumping this in your inbox. Please have a look at above questions when you have a chance.

@shravanrn
Copy link
Collaborator Author

@sbc100 @keithw Have made all the changes, could you please sign off on the review

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks! I like this method much more, personally

@@ -0,0 +1,176 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

How about just calling calling these wasm-rt-impl-mem.c (so they share a common prefix with wasm-rt-impl.c?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... this would make it inconsistent with exceptions which is in wasm-rt-exceptions-impl.c. Happy to go with either, but we can maybe do this rename in a follow up PR to since we have to change multiple things

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.

4 participants