154704 2016-02-25 16:54:58 -0800 [JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero 2016-02-27 15:10:27 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 benjamin benjamin commit-queue fpizlo keith_miller mark.lam msaboff saam oldest_to_newest 1168304 0 benjamin 2016-02-25 16:54:58 -0800 [JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero 1168316 1 272266 benjamin 2016-02-25 17:17:46 -0800 Created attachment 272266 Patch 1168317 2 commit-queue 2016-02-25 17:19:04 -0800 Attachment 272266 did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:431: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style. 1168443 3 272266 ggaren 2016-02-26 00:15:00 -0800 Comment on attachment 272266 Patch r=me 1168598 4 272266 commit-queue 2016-02-26 11:18:50 -0800 Comment on attachment 272266 Patch Clearing flags on attachment: 272266 Committed r197186: <http://trac.webkit.org/changeset/197186> 1168599 5 commit-queue 2016-02-26 11:18:55 -0800 All reviewed patches have been landed. Closing bug. 1168907 6 272266 fpizlo 2016-02-27 15:09:51 -0800 Comment on attachment 272266 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272266&action=review > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:204 > void add32(TrustedImm32 imm, RegisterID src, RegisterID dest) > { > if (!imm.m_value) { > - move(src, dest); > + zeroExtend32ToPtr(src, dest); > return; > } > I think that this is still wrong. Can we please remove this strength reduction rule from the MacroAssembler? Unless a move instruction sets the same flags as an add32 instruction, then this constant-folding rule will cause us great confusion if we ever start reasoning about flags. Reasoning about flags unlocks smart things, like realizing that if you compare the result of an addition to zero, you can usually just remove the compare (or test) instruction and branch directly on add's flags. I've already removed all other places that I found where we did this !imm.m_value optimization, because they were also causing ZDef bugs. I then removed the add32AndSetFlags() thing because that appeared to mostly be for avoiding this optimization. So, now the expectation is that add32() sets flags. 1168908 7 fpizlo 2016-02-27 15:10:27 -0800 (In reply to comment #6) > Comment on attachment 272266 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272266&action=review > > > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:204 > > void add32(TrustedImm32 imm, RegisterID src, RegisterID dest) > > { > > if (!imm.m_value) { > > - move(src, dest); > > + zeroExtend32ToPtr(src, dest); > > return; > > } > > > > I think that this is still wrong. Can we please remove this strength > reduction rule from the MacroAssembler? Unless a move instruction sets the > same flags as an add32 instruction, then this constant-folding rule will > cause us great confusion if we ever start reasoning about flags. Reasoning > about flags unlocks smart things, like realizing that if you compare the > result of an addition to zero, you can usually just remove the compare (or > test) instruction and branch directly on add's flags. > > I've already removed all other places that I found where we did this > !imm.m_value optimization, because they were also causing ZDef bugs. I then > removed the add32AndSetFlags() thing because that appeared to mostly be for > avoiding this optimization. So, now the expectation is that add32() sets > flags. I just realized that I'm wrong - three-operand add32() on X86 doesn't set flags! Never mind everything I said above. 272266 2016-02-25 17:17:46 -0800 2016-02-26 11:18:50 -0800 Patch bug-154704-20160225171729.patch text/plain 2953 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTk3MTM1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA1 ZjcyZDljYjAyOTkzMzBkYjBkYTBkNGJiNGQ4YmI1YTU5MjBlNTAwLi44N2RkOWZhOTQ0MDU5YzVl NDU5YWQyN2M4NTNjMjQ1ODJhODVjY2Q0IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs NSArMSwxOSBAQAogMjAxNi0wMi0yNSAgQmVuamFtaW4gUG91bGFpbiAgPGJwb3VsYWluQGFwcGxl LmNvbT4KIAorICAgICAgICBbSlNDXSBBZGQzMihJbW0sIFRtcCwgVG1wKSBkb2VzIG5vdCBaRGVm IHRoZSBkZXN0aW5hdGlvbiBpZiBJbW0gaXMgemVybworICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU0NzA0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgSWYgdGhlIEltbSBpcyB6ZXJvLCB3ZSBzaG91bGQgc3Rp bGwgemVybyB0aGUgdG9wIGJpdHMKKyAgICAgICAgdG8gbWF0Y2ggdGhlIGRlZmluaXRpb24gaW4g QWlyT3Bjb2Rlcy4KKworICAgICAgICAqIGFzc2VtYmxlci9NYWNyb0Fzc2VtYmxlclg4NkNvbW1v bi5oOgorICAgICAgICAoSlNDOjpNYWNyb0Fzc2VtYmxlclg4NkNvbW1vbjo6YWRkMzIpOgorICAg ICAgICAqIGIzL3Rlc3RiMy5jcHA6CisKKzIwMTYtMDItMjUgIEJlbmphbWluIFBvdWxhaW4gIDxi cG91bGFpbkBhcHBsZS5jb20+CisKICAgICAgICAgW0pTQ10gUmVtb3ZlIGEgdXNlbGVzcyAiTW92 ZSIgaW4gdGhlIGxvd2VyaW5nIG9mIFNlbGVjdAogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU0NjcwCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJYODZDb21tb24uaCBiL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJYODZDb21tb24uaAppbmRleCAzZWY5 ODg1NjQxNTI2OGZkOTJhODA4MzU4MGY4ODZmZDE5OWM1ZDZjLi4wOGVhNTc1OWQ3ZDkxZGQ2MThj NDg5YmIxOGZhNTM4MWYzODA3NDA5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv YXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyWDg2Q29tbW9uLmgKKysrIGIvU291cmNlL0phdmFTY3Jp cHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlclg4NkNvbW1vbi5oCkBAIC0xOTgsNyArMTk4 LDcgQEAgcHVibGljOgogICAgIHZvaWQgYWRkMzIoVHJ1c3RlZEltbTMyIGltbSwgUmVnaXN0ZXJJ RCBzcmMsIFJlZ2lzdGVySUQgZGVzdCkKICAgICB7CiAgICAgICAgIGlmICghaW1tLm1fdmFsdWUp IHsKLSAgICAgICAgICAgIG1vdmUoc3JjLCBkZXN0KTsKKyAgICAgICAgICAgIHplcm9FeHRlbmQz MlRvUHRyKHNyYywgZGVzdCk7CiAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgIH0KIApkaWZm IC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2IzL3Rlc3RiMy5jcHAgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvYjMvdGVzdGIzLmNwcAppbmRleCAyZWMzMWY4OTU4NmU0YzNiOGFlNjdmYTI5 NGVhYTBkMzBlN2I0ODc5Li42MzE1NDNlNjJiZWU3MzdmOGRkNDcwYzZiMmFmNGRkMGI4ZWM1NDdm IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYjMvdGVzdGIzLmNwcAorKysgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvYjMvdGVzdGIzLmNwcApAQCAtNDEyLDYgKzQxMiwyNSBAQCB2 b2lkIHRlc3RBZGRJbW1NZW0zMihpbnQzMl90IGEsIGludDMyX3QgYikKICAgICBDSEVDSyhpbnB1 dE91dHB1dCA9PSBhICsgYik7CiB9CiAKK3ZvaWQgdGVzdEFkZEFyZ1plcm9JbW1aRGVmKCkKK3sK KyAgICBQcm9jZWR1cmUgcHJvYzsKKyAgICBCYXNpY0Jsb2NrKiByb290ID0gcHJvYy5hZGRCbG9j aygpOworICAgIFZhbHVlKiBhcmcgPSByb290LT5hcHBlbmROZXc8VmFsdWU+KAorICAgICAgICBw cm9jLCBUcnVuYywgT3JpZ2luKCksCisgICAgICAgIHJvb3QtPmFwcGVuZE5ldzxBcmd1bWVudFJl Z1ZhbHVlPihwcm9jLCBPcmlnaW4oKSwgR1BSSW5mbzo6YXJndW1lbnRHUFIwKSk7CisgICAgVmFs dWUqIGNvbnN0WmVybyA9IHJvb3QtPmFwcGVuZE5ldzxDb25zdDMyVmFsdWU+KHByb2MsIE9yaWdp bigpLCAwKTsKKyAgICByb290LT5hcHBlbmROZXc8Q29udHJvbFZhbHVlPigKKyAgICAgICAgcHJv YywgUmV0dXJuLCBPcmlnaW4oKSwKKyAgICAgICAgcm9vdC0+YXBwZW5kTmV3PFZhbHVlPigKKyAg ICAgICAgICAgIHByb2MsIEFkZCwgT3JpZ2luKCksCisgICAgICAgICAgICBhcmcsCisgICAgICAg ICAgICBjb25zdFplcm8pKTsKKworICAgIGF1dG8gY29kZSA9IGNvbXBpbGUocHJvYywgMCk7Cisg ICAgQ0hFQ0soaW52b2tlPGludDY0X3Q+KCpjb2RlLCAweDAxMjM0NTY3ODlhYmNkZWYpID09IDB4 ODlhYmNkZWYpOworfQorCiB2b2lkIHRlc3RBZGRMb2FkVHdpY2UoKQogewogICAgIGF1dG8gdGVz dCA9IFsmXSAodW5zaWduZWQgb3B0TGV2ZWwpIHsKQEAgLTEwNjE1LDYgKzEwNjM0LDcgQEAgdm9p ZCBydW4oY29uc3QgY2hhciogZmlsdGVyKQogICAgIFJVTl9CSU5BUlkodGVzdEFkZEFyZ01lbTMy LCBpbnQzMk9wZXJhbmRzKCksIGludDMyT3BlcmFuZHMoKSk7CiAgICAgUlVOX0JJTkFSWSh0ZXN0 QWRkTWVtQXJnMzIsIGludDMyT3BlcmFuZHMoKSwgaW50MzJPcGVyYW5kcygpKTsKICAgICBSVU5f QklOQVJZKHRlc3RBZGRJbW1NZW0zMiwgaW50MzJPcGVyYW5kcygpLCBpbnQzMk9wZXJhbmRzKCkp OworICAgIFJVTih0ZXN0QWRkQXJnWmVyb0ltbVpEZWYoKSk7CiAgICAgUlVOKHRlc3RBZGRMb2Fk VHdpY2UoKSk7CiAKICAgICBSVU4odGVzdEFkZEFyZ0RvdWJsZShNX1BJKSk7Cg==