{"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":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-06-20T13:54:25Z","updated_at":"2016-06-20T18:39:45Z","closed_at":null,"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/227230894","html_url":"https://github.com/brson/mir2wasm/pull/9#issuecomment-227230894","issue_url":"https://api.github.com/repos/brson/mir2wasm/issues/9","id":227230894,"user":{"login":"brson","id":147214,"avatar_url":"https://avatars.githubusercontent.com/u/147214?v=3","gravatar_id":"","url":"https://api.github.com/users/brson","html_url":"https://github.com/brson","followers_url":"https://api.github.com/users/brson/followers","following_url":"https://api.github.com/users/brson/following{/other_user}","gists_url":"https://api.github.com/users/brson/gists{/gist_id}","starred_url":"https://api.github.com/users/brson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brson/subscriptions","organizations_url":"https://api.github.com/users/brson/orgs","repos_url":"https://api.github.com/users/brson/repos","events_url":"https://api.github.com/users/brson/events{/privacy}","received_events_url":"https://api.github.com/users/brson/received_events","type":"User","site_admin":false},"created_at":"2016-06-20T18:39:45Z","updated_at":"2016-06-20T18:39:45Z","body":"> First, 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\nI don't know enough about this to say. In general this code is so early that anything that works is good for progress.\r\n\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\r\nI guess this table should be keyed by binaryen types? Seems good to me, though wouldn't we have to create the duplicate binaryen type in order to discover that it hashes to the same value as one we've already seen (defeating the potential optimization)?\r\n\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\r\nFor primitive types rustc emits the instructions directly and it doesn't matter whether the ops are actually defined, so what you are doing is fine for now.\r\n\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\r\nPerhaps @eddyb @nagisa @solson or @Aatch could help here.\r\n\r\nRelatedly, should mir2wasm move to using the item collector now and not deal with monomorphisations itself? Do you have pointers on how to use it?\r\n\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. (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)\r\n\r\nIt would be great to have a plan. Unfortunately, I don't know enough about how either compiler works to know what the steps are. My plan has been to hack things one step at a time by reading the rustc source and bugging the aforementioned people on #rust-internals. Maybe one of them could lay out a vague roadmap.\r\n\r\nI would guess that the immediate steps would be to update to Rust master and move to using the item collector to deal with monomorphization (handling monomorphization is one of the biggest sources of ugliness in rustc's old backend, but its new backend has a cleaner way).\r\n"}}
{"actor":{"display_login":"brson"}}