{"action":"created","issue":{"url":"https://api.github.com/repos/brson/mir2wasm/issues/9","repository_url":"https://api.github.com/repos/brson/mir2wasm","labels_url":"https://api.github.com/repos/brson/mir2wasm/issues/9/labels{/name}","comments_url":"https://api.github.com/repos/brson/mir2wasm/issues/9/comments","events_url":"https://api.github.com/repos/brson/mir2wasm/issues/9/events","html_url":"https://github.com/brson/mir2wasm/pull/9","id":161200390,"number":9,"title":"Add sub, mul, and div operators","user":{"login":"lqd","id":247183,"avatar_url":"https://avatars.githubusercontent.com/u/247183?v=3","gravatar_id":"","url":"https://api.github.com/users/lqd","html_url":"https://github.com/lqd","followers_url":"https://api.github.com/users/lqd/followers","following_url":"https://api.github.com/users/lqd/following{/other_user}","gists_url":"https://api.github.com/users/lqd/gists{/gist_id}","starred_url":"https://api.github.com/users/lqd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lqd/subscriptions","organizations_url":"https://api.github.com/users/lqd/orgs","repos_url":"https://api.github.com/users/lqd/repos","events_url":"https://api.github.com/users/lqd/events{/privacy}","received_events_url":"https://api.github.com/users/lqd/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":11,"created_at":"2016-06-20T13:54:25Z","updated_at":"2016-06-22T19:51:26Z","closed_at":"2016-06-21T01:14:50Z","pull_request":{"url":"https://api.github.com/repos/brson/mir2wasm/pulls/9","html_url":"https://github.com/brson/mir2wasm/pull/9","diff_url":"https://github.com/brson/mir2wasm/pull/9.diff","patch_url":"https://github.com/brson/mir2wasm/pull/9.patch"},"body":"This PR adds those 3 operators, and also asks a lot of questions :)\r\n\r\nAn example of the wast output (manually modified to import `print`, call it inside the module, and call the exported test function so that I could check the output in binaryen-shell's interpreter): https://gist.github.com/lqd/44f627530413df83b27815808b953356\r\n\r\nFirst, a comment: I'm not sure about Div. I emitted `i32.div` at first but that made the binaryen checker/interpreter trap, so I switched to `i32.div_s` which worked (maybe @kripken could weigh in on this choice ? and I will modify accordingly).\r\n\r\nSecond: adding the operators triggered some questions/thoughts\r\n- The fun_types are a set of rust sigs, however those will be converted to wasm func types, and thus will emit duplicates. Probably not that big of a deal, considering binaryen's optimization passes remove a lot of inefficient wasm patterns (even more so in master than the version used in mir2wasm) even though this specific inefficiency of duplicates and unused func types is not yet handled in binaryen IIRC.\r\n- Rust requires those traits & impls to compile, but those operators are wasm \"intrinsics\", the rust operators' code translated to wasm isn't used (and maybe shouldn't be emitted ? surely not a big deal either, binaryen will optimize them away)\r\n- I was working on/fighting with PartialEq: in mir2wasm a lookup was done on `PartialEq::eq` (eventually, in `trans_fn_name_direct`) but the impls in the module-to-translate were more like `<isize_as_PartialEq>::eq`  -- how should this be handled ? (In my own code I hacked around this by returning the isize PartialEq::eq node id when seeing the PartialEq fn defid, but I doubt very much this is how you guys thought it should work :) -- All in all, this personally allowed me to get back to the BinOp Eq/Ne path and make it work, as well as the AddAssign trait)\r\n- is there a plan on how the missing things in mir2wasm should work ? and at the same time maybe some \"issues\" would be interesting, so that we see the things you guys could use some help with ? I talked a bit about that on twitter with @kripken yesterday.\r\n(In my mind I was thinking mir2wasm would maybe use some of emscripten's design to model high level things -- stack frames, memory access, data segments, and so on -- and was waiting for those building blocks to appear before myself contributing on some different topics, having this \"plan\" and those issues would be helpful in that regard)"},"comment":{"url":"https://api.github.com/repos/brson/mir2wasm/issues/comments/227856974","html_url":"https://github.com/brson/mir2wasm/pull/9#issuecomment-227856974","issue_url":"https://api.github.com/repos/brson/mir2wasm/issues/9","id":227856974,"user":{"login":"kripken","id":173661,"avatar_url":"https://avatars.githubusercontent.com/u/173661?v=3","gravatar_id":"","url":"https://api.github.com/users/kripken","html_url":"https://github.com/kripken","followers_url":"https://api.github.com/users/kripken/followers","following_url":"https://api.github.com/users/kripken/following{/other_user}","gists_url":"https://api.github.com/users/kripken/gists{/gist_id}","starred_url":"https://api.github.com/users/kripken/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kripken/subscriptions","organizations_url":"https://api.github.com/users/kripken/orgs","repos_url":"https://api.github.com/users/kripken/repos","events_url":"https://api.github.com/users/kripken/events{/privacy}","received_events_url":"https://api.github.com/users/kripken/received_events","type":"User","site_admin":false},"created_at":"2016-06-22T19:51:26Z","updated_at":"2016-06-22T19:51:26Z","body":"Yes, in latest binaryen, there is a type on those operations, to remove ambiguity. The old `BinaryenAdd` could work for all types, and the new way is to use `BinaryenAddInt32`, for `i32` values, etc.\r\n"}}
{"actor":{"display_login":"kripken"}}