Added atomic ops to binaryen.js as well#1280
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Otherwise this looks good, but please add a test.
|
|
||
| function makeAtomicOps(ofType) { | ||
| return { | ||
| 'rmw': { |
There was a problem hiding this comment.
this could perhaps be more concise, since the return [..] line is almost identical on all these except one. So we could switch on just the add => Module['AtomicRMWAdd'] parts, what do you think?
There was a problem hiding this comment.
Wasn't sure if there might be additional atomic ops in the future, besides rmw. Other than that, we'd also have to repeat the { 'rmw': ... } four times below then - or did you mean to remove the rmw subsection entirely?
There was a problem hiding this comment.
Not sure what you mean by repeating that section, but maybe I wasn't clear before. This is what I meant, just to remove the duplication in those rmws by changing makeAtomicOps to this: https://gist.github.com/kripken/406fa34ab78a608a3442aaa8b0e77a40
There was a problem hiding this comment.
I see, thanks. This could be done for other functions as well. Though, the updated commit now uses the usual format similar to the other functions. Suggesting to leave the refactoring for another PR.
|
There are no f32/f64 atomic ops in wasm currently, so it's probably best not to add these to binaryen. |
dcodeIO
force-pushed
the
js-atomic-ops
branch
from
c53022b to
10c394e
Compare
|
Sorry, had a bad start on this one. Should actually stick to the text format now and implement supported ops only. |
3 participants
As a follow-up to #1276 being merged, this PR adds the following methods to binaryen.js:
Each method is named after its text format representation (I used
Print.cppas a reference) and takes the same parameters as its respective C-API function, withmodule,op,bytes,type,expectedType(if applicable) substituted accordingly.