207849 2020-02-17 11:12:05 -0800 [Wasm] REGRESSION(r256665): Wasm->JS call IC needs to save memory size register 2020-02-17 13:24:19 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 tzagallo tzagallo commit-queue ews-watchlist keith_miller Lawrence.j mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1619745 0 tzagallo 2020-02-17 11:12:05 -0800 ... 1619748 1 390944 tzagallo 2020-02-17 11:16:58 -0800 Created attachment 390944 Patch 1619749 2 tzagallo 2020-02-17 11:19:01 -0800 <rdar://problem/59504154> 1619770 3 390944 mark.lam 2020-02-17 11:54:07 -0800 Comment on attachment 390944 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390944&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:165 > + // Pessimistically save callee saves in BoundsChecking mode since the LLInt always bounds checks > + return Wasm::PinnedRegisterInfo::get().toSave(Wasm::MemoryMode::BoundsChecking); When would we ever not want to save the sizeRegister? Why not get rid of the memoryMode condition and always save the register? From gripping the code, I don't see a case where toSave() is ever called with anything other than MemoryMode::BoundsChecking. 1619771 4 mark.lam 2020-02-17 11:57:36 -0800 (In reply to Mark Lam from comment #3) > > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:165 > > + // Pessimistically save callee saves in BoundsChecking mode since the LLInt always bounds checks > > + return Wasm::PinnedRegisterInfo::get().toSave(Wasm::MemoryMode::BoundsChecking); > > When would we ever not want to save the sizeRegister? Why not get rid of > the memoryMode condition and always save the register? From gripping the > code, I don't see a case where toSave() is ever called with anything other > than MemoryMode::BoundsChecking. /gripping/grepping/ 1619805 5 390944 mark.lam 2020-02-17 12:42:25 -0800 Comment on attachment 390944 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390944&action=review r=me >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:165 >> + return Wasm::PinnedRegisterInfo::get().toSave(Wasm::MemoryMode::BoundsChecking); > > When would we ever not want to save the sizeRegister? Why not get rid of the memoryMode condition and always save the register? From gripping the code, I don't see a case where toSave() is ever called with anything other than MemoryMode::BoundsChecking. I'm wrong, there's still one place where we may not pass MemoryMode::BoundsChecking. 1619832 6 tzagallo 2020-02-17 13:15:14 -0800 *** Bug 207843 has been marked as a duplicate of this bug. *** 1619836 7 390944 commit-queue 2020-02-17 13:24:18 -0800 Comment on attachment 390944 Patch Clearing flags on attachment: 390944 Committed r256766: <https://trac.webkit.org/changeset/256766> 1619837 8 commit-queue 2020-02-17 13:24:19 -0800 All reviewed patches have been landed. Closing bug. 390944 2020-02-17 11:16:58 -0800 2020-02-17 13:24:18 -0800 Patch bug-207849-20200217111657.patch text/plain 3306 tzagallo U3VidmVyc2lvbiBSZXZpc2lvbjogMjU2NzM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj ZDc5NzZiMmM5NDRiNDA5OWU1NWViZWU5ZjA5NzBkNWM2MDVhODQ0Li4wNjk4NWRkNjlkZGIxY2Ji YTE3MzY3ZjljZDI4MzIxYmZkYzRkZGNiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAyMC0wMi0xNyAgVGFkZXUgWmFnYWxsbyAgPHR6YWdhbGxvQGFwcGxlLmNv bT4KKworICAgICAgICBbV2FzbV0gUkVHUkVTU0lPTihyMjU2NjY1KTogV2FzbS0+SlMgY2FsbCBJ QyBuZWVkcyB0byBzYXZlIG1lbW9yeSBzaXplIHJlZ2lzdGVyCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDc4NDkKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXaGVuIGdlbmVyYXRpbmcgdGhlIGNhbGwgSUMs IHdlIHNob3VsZCBzZWxlY3QgdGhlIGNhbGxlZSBzYXZlcyB1c2luZyBCb3VuZHNDaGVja2luZyBt b2RlIGluIG9yZGVyCisgICAgICAgIHRvIG9iZXkgdG8gdGhlIGNhbGxpbmcgY29udmVudGlvbnMg ZGVzY3JpYmVkIGluIHIyNTY2NjUuIEN1cnJlbnRseSwgd2Ugd29uJ3QgcmVzdG9yZSB0aGUgbWVt b3J5IHNpemUKKyAgICAgICAgcmVnaXN0ZXIgd2hlbiBjYWxsaW5nIHRoZSBXYXNtIExMSW50IHRo cm91Z2ggdGhlIGNhbGwgSUMuCisKKyAgICAgICAgKiB3YXNtL2pzL1dlYkFzc2VtYmx5RnVuY3Rp b24uY3BwOgorICAgICAgICAoSlNDOjpXZWJBc3NlbWJseUZ1bmN0aW9uOjpjYWxsZWVTYXZlcyBj b25zdCk6CisKIDIwMjAtMDItMTYgIEZ1amlpIEhpcm9ub3JpICA8SGlyb25vcmkuRnVqaWlAc29u eS5jb20+CiAKICAgICAgICAgUmVtb3ZlIHJlbWFpbmluZyBXVEZfRVhQT1JUIGFuZCBXVEZfSU1Q T1JUIGJ5IHJlcGxhY2luZyB0aGVtIHdpdGggV1RGX0VYUE9SVF9ERUNMQVJBVElPTiBhbmQgV1RG X0lNUE9SVF9ERUNMQVJBVElPTgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3dh c20vanMvV2ViQXNzZW1ibHlGdW5jdGlvbi5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd2Fz bS9qcy9XZWJBc3NlbWJseUZ1bmN0aW9uLmNwcAppbmRleCA0NGY5OWJlYzk5NzJiNTc5MmVjMWI4 NzE0MzkzMzcwMDIwYWIzZTY1Li4xNmVjZGFhNjFlODZlYzg4ZGUwNDQ4ZTI5ZjY2NTg2MDY2ZWM4 NGExIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd2FzbS9qcy9XZWJBc3NlbWJs eUZ1bmN0aW9uLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd2FzbS9qcy9XZWJBc3Nl bWJseUZ1bmN0aW9uLmNwcApAQCAtMTYxLDcgKzE2MSw4IEBAIGJvb2wgV2ViQXNzZW1ibHlGdW5j dGlvbjo6dXNlVGFnUmVnaXN0ZXJzKCkgY29uc3QKIAogUmVnaXN0ZXJTZXQgV2ViQXNzZW1ibHlG dW5jdGlvbjo6Y2FsbGVlU2F2ZXMoKSBjb25zdAogewotICAgIHJldHVybiBXYXNtOjpQaW5uZWRS ZWdpc3RlckluZm86OmdldCgpLnRvU2F2ZShpbnN0YW5jZSgpLT5tZW1vcnlNb2RlKCkpOworICAg IC8vIFBlc3NpbWlzdGljYWxseSBzYXZlIGNhbGxlZSBzYXZlcyBpbiBCb3VuZHNDaGVja2luZyBt b2RlIHNpbmNlIHRoZSBMTEludCBhbHdheXMgYm91bmRzIGNoZWNrcworICAgIHJldHVybiBXYXNt OjpQaW5uZWRSZWdpc3RlckluZm86OmdldCgpLnRvU2F2ZShXYXNtOjpNZW1vcnlNb2RlOjpCb3Vu ZHNDaGVja2luZyk7CiB9CiAKIFJlZ2lzdGVyQXRPZmZzZXRMaXN0IFdlYkFzc2VtYmx5RnVuY3Rp b246OnVzZWRDYWxsZWVTYXZlUmVnaXN0ZXJzKCkgY29uc3QKZGlmZiAtLWdpdCBhL0pTVGVzdHMv Q2hhbmdlTG9nIGIvSlNUZXN0cy9DaGFuZ2VMb2cKaW5kZXggNjUyMTJmZmExZDc1YTk0MTY2MWYz NzZjOWQyZWMwNTk1MmYzYjg2OC4uN2M2YmMwNjBiMDFiMjFjNzhkZDAyNmQ1MzEyZGM2MzUzMTcy MmUwNCAxMDA2NDQKLS0tIGEvSlNUZXN0cy9DaGFuZ2VMb2cKKysrIGIvSlNUZXN0cy9DaGFuZ2VM b2cKQEAgLTEsMyArMSwxMyBAQAorMjAyMC0wMi0xNyAgVGFkZXUgWmFnYWxsbyAgPHR6YWdhbGxv QGFwcGxlLmNvbT4KKworICAgICAgICBbV2FzbV0gUkVHUkVTU0lPTihyMjU2NjY1KTogV2FzbS0+ SlMgY2FsbCBJQyBuZWVkcyB0byBzYXZlIG1lbW9yeSBzaXplIHJlZ2lzdGVyCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDc4NDkKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHdhc20vcmVncmVzcy9yZWdy ZXNzLTI1NjY2NS5qczogQWRkZWQuCisgICAgICAgIChmKToKKwogMjAyMC0wMi0xNCAgVGFkZXUg WmFnYWxsbyAgPHR6YWdhbGxvQGFwcGxlLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkOiBmaXgg YnJva2VuIHRlc3RzIGFkZGVkIGluIHIyNTY2NjUKZGlmZiAtLWdpdCBhL0pTVGVzdHMvd2FzbS9y ZWdyZXNzL3JlZ3Jlc3MtMjU2NjY1LmpzIGIvSlNUZXN0cy93YXNtL3JlZ3Jlc3MvcmVncmVzcy0y NTY2NjUuanMKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMC4uM2FiNmMxNmMxNTc5NWRiNmZhNzgwNWFhYjY0NGUwNzE3YWQ0 ZjJhNAotLS0gL2Rldi9udWxsCisrKyBiL0pTVGVzdHMvd2FzbS9yZWdyZXNzL3JlZ3Jlc3MtMjU2 NjY1LmpzCkBAIC0wLDAgKzEsMTIgQEAKKy8vQCByZXF1aXJlT3B0aW9ucygiLS11c2VDb25jdXJy ZW50SklUPWZhbHNlIiwgIi0taml0UG9saWN5U2NhbGU9MCIpCisKK2Z1bmN0aW9uIGYoKSB7Cisg ICAgdmFyIGJ1ZmZlciA9IG5ldyBVaW50OEFycmF5KFswLCA5NywgMTE1LCAxMDksIDEsIDAsIDAs IDAsIDEsIDYsIDEsIDk2LCAxLCAxMjcsIDEsIDEyNywgMywgMiwgMSwgMCwgNSwgMywgMSwgMCwg MCwgNywgOCwgMSwgNCwgMTA4LCAxMTEsIDk3LCAxMDAsIDAsIDAsIDEwLCA5LCAxLCA3LCAwLCAz MiwgMCwgNDAsIDAsIDEwMCwgMTFdKTsKKyAgICB2YXIgbW9kdWxlID0gbmV3IFdlYkFzc2VtYmx5 Lk1vZHVsZShidWZmZXIpOworICAgIHZhciBpbnN0YW5jZSA9IG5ldyBXZWJBc3NlbWJseS5JbnN0 YW5jZShtb2R1bGUpOworICAgIHRyeSB7IGluc3RhbmNlLmV4cG9ydHMubG9hZCgweDEwMDAwIC0g MTAwIC0gNCk7IH0gY2F0Y2ggKGUpIHt9CisgICAgKDU1NSlbMF07Cit9CisKK2YoKTsKK2YoKTsK