152184 2015-12-11 13:24:03 -0800 B3::reduceStrength should remove redundant Phi's 2015-12-11 16:18:08 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build All All RESOLVED FIXED P2 Normal --- 150279 1 fpizlo fpizlo commit-queue keith_miller mark.lam msaboff saam oldest_to_newest 1148521 0 fpizlo 2015-12-11 13:24:03 -0800 Patch forthcoming. 1148525 1 267184 fpizlo 2015-12-11 13:32:11 -0800 Created attachment 267184 the patch 1148554 2 267184 benjamin 2015-12-11 14:49:54 -0800 Comment on attachment 267184 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=267184&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1252 > + if (otherChild == nullptr) { if (!otherChild) > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1265 > + // Wow, this would be super weird. It probably won't happen, except that things could > + // get weird as a consequence of stepwise simplifications in the strength reduction > + // fixpoint. > + continue; We can only get here in two ways: 1) There are no values for this Phi 2) All the values are the Phi itself. [2] would be the sign of an invalid SSA form. [1] should have been eliminated by killDeadCode(). Wouldn't an ASSER_WITH_MESSAGE() make sense here? 1148561 3 fpizlo 2015-12-11 15:07:26 -0800 (In reply to comment #2) > Comment on attachment 267184 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267184&action=review > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1252 > > + if (otherChild == nullptr) { > > if (!otherChild) > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1265 > > + // Wow, this would be super weird. It probably won't happen, except that things could > > + // get weird as a consequence of stepwise simplifications in the strength reduction > > + // fixpoint. > > + continue; > > We can only get here in two ways: > 1) There are no values for this Phi > 2) All the values are the Phi itself. > > [2] would be the sign of an invalid SSA form. > > [1] should have been eliminated by killDeadCode(). > > Wouldn't an ASSER_WITH_MESSAGE() make sense here? All of the values could be the Phi itself if this is a loop that became unreachable. I don't see how it would happen, but in my experience all of these kinds of cases where we add an assertion even though we could have handled it with an early return mean that eventually we just remove the assertion. Better to remove it now, so that if we change the ordering of the fixpoint, we don't have to worry about this assertion. 1148600 4 fpizlo 2015-12-11 16:18:08 -0800 Landed in http://trac.webkit.org/changeset/193993 267184 2015-12-11 13:32:11 -0800 2015-12-11 14:49:54 -0800 the patch blah.patch text/plain 6534 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTkzOTczKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBA CisyMDE1LTEyLTExICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg QjM6OnJlZHVjZVN0cmVuZ3RoIHNob3VsZCByZW1vdmUgcmVkdW5kYW50IFBoaSdzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTIxODQKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIGFkZHMgcmVkdW5k YW50IFBoaSByZW1vdmFsIHVzaW5nIEF5Y29jayBhbmQgSG9yc3Bvb2xzIFNTQSBzaW1wbGlmaWNh dGlvbiBhbGdvcml0aG0uIFRoaXMKKyAgICAgICAgaXMgbmVlZGVkIGJlY2F1c2UgZXZlbiBpbiBz aW1wbGUgYXNtLmpzIGNvZGUsIHdlIHNlZSBhIGxvdCBvZiBDRkcgc2ltcGxpZmljYXRpb24gdGhh dCBsZWF2ZXMKKyAgICAgICAgYmVoaW5kIHRvdGFsbHkgdXNlbGVzcyBQaGkncy4KKworICAgICAg ICAqIGIzL0IzUGhpQ2hpbGRyZW4uY3BwOgorICAgICAgICAoSlNDOjpCMzo6UGhpQ2hpbGRyZW46 OlBoaUNoaWxkcmVuKToKKyAgICAgICAgKiBiMy9CM1BoaUNoaWxkcmVuLmg6CisgICAgICAgIChK U0M6OkIzOjpQaGlDaGlsZHJlbjo6YXQpOgorICAgICAgICAoSlNDOjpCMzo6UGhpQ2hpbGRyZW46 Om9wZXJhdG9yW10pOgorICAgICAgICAoSlNDOjpCMzo6UGhpQ2hpbGRyZW46OnBoaXMpOgorICAg ICAgICAqIGIzL0IzUmVkdWNlU3RyZW5ndGguY3BwOgorCiAyMDE1LTEyLTExICBKb3NlcGggUGVj b3Jhcm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CiAKICAgICAgICAgY2hlY2stZm9yLWluYXBwcm9w cmlhdGUtb2JqYy1jbGFzcy1uYW1lcyBzaG91bGQgY2hlY2sgYWxsIGNsYXNzIG5hbWVzLCBub3Qg anVzdCBleHRlcm5hbGx5IHZpc2libGUgb25lcwpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3Jl L2IzL0IzUGhpQ2hpbGRyZW4uY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9iMy9CM1BoaUNoaWxkcmVuLmNwcAkocmV2aXNpb24gMTkzOTQ2KQorKysgU291cmNlL0phdmFT Y3JpcHRDb3JlL2IzL0IzUGhpQ2hpbGRyZW4uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNiw4ICsz NiwxMyBAQCBQaGlDaGlsZHJlbjo6UGhpQ2hpbGRyZW4oUHJvY2VkdXJlJiBwcm9jCiAgICAgOiBt X3Vwc2lsb25zKHByb2MudmFsdWVzKCkuc2l6ZSgpKQogewogICAgIGZvciAoVmFsdWUqIHZhbHVl IDogcHJvYy52YWx1ZXMoKSkgewotICAgICAgICBpZiAoVXBzaWxvblZhbHVlKiB1cHNpbG9uID0g dmFsdWUtPmFzPFVwc2lsb25WYWx1ZT4oKSkKLSAgICAgICAgICAgIG1fdXBzaWxvbnNbdXBzaWxv bi0+cGhpKCldLmFwcGVuZCh1cHNpbG9uKTsKKyAgICAgICAgaWYgKFVwc2lsb25WYWx1ZSogdXBz aWxvbiA9IHZhbHVlLT5hczxVcHNpbG9uVmFsdWU+KCkpIHsKKyAgICAgICAgICAgIFZhbHVlKiBw aGkgPSB1cHNpbG9uLT5waGkoKTsKKyAgICAgICAgICAgIFZlY3RvcjxVcHNpbG9uVmFsdWUqPiYg dmVjdG9yID0gbV91cHNpbG9uc1twaGldOworICAgICAgICAgICAgaWYgKHZlY3Rvci5pc0VtcHR5 KCkpCisgICAgICAgICAgICAgICAgbV9waGlzLmFwcGVuZChwaGkpOworICAgICAgICAgICAgdmVj dG9yLmFwcGVuZCh1cHNpbG9uKTsKKyAgICAgICAgfQogICAgIH0KIH0KIApJbmRleDogU291cmNl L0phdmFTY3JpcHRDb3JlL2IzL0IzUGhpQ2hpbGRyZW4uaAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvYjMvQjNQaGlDaGlsZHJlbi5oCShyZXZpc2lvbiAxOTM5NDYpCisrKyBT b3VyY2UvSmF2YVNjcmlwdENvcmUvYjMvQjNQaGlDaGlsZHJlbi5oCSh3b3JraW5nIGNvcHkpCkBA IC0xNjYsOCArMTY2LDExIEBAIHB1YmxpYzoKICAgICBVcHNpbG9uQ29sbGVjdGlvbiBhdChWYWx1 ZSogdmFsdWUpIHsgcmV0dXJuIFVwc2lsb25Db2xsZWN0aW9uKHRoaXMsIHZhbHVlLCAmbV91cHNp bG9uc1t2YWx1ZV0pOyB9CiAgICAgVXBzaWxvbkNvbGxlY3Rpb24gb3BlcmF0b3JbXShWYWx1ZSog dmFsdWUpIHsgcmV0dXJuIGF0KHZhbHVlKTsgfQogCisgICAgY29uc3QgVmVjdG9yPFZhbHVlKiwg OD4mIHBoaXMoKSBjb25zdCB7IHJldHVybiBtX3BoaXM7IH0KKwogcHJpdmF0ZToKICAgICBJbmRl eE1hcDxWYWx1ZSwgVmVjdG9yPFVwc2lsb25WYWx1ZSo+PiBtX3Vwc2lsb25zOworICAgIFZlY3Rv cjxWYWx1ZSosIDg+IG1fcGhpczsKIH07CiAKIH0gfSAvLyBuYW1lc3BhY2UgSlNDOjpCMwpJbmRl eDogU291cmNlL0phdmFTY3JpcHRDb3JlL2IzL0IzUmVkdWNlU3RyZW5ndGguY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9iMy9CM1JlZHVjZVN0cmVuZ3RoLmNwcAkocmV2 aXNpb24gMTkzOTQ2KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2IzL0IzUmVkdWNlU3RyZW5n dGguY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNCw2ICszNCw3IEBACiAjaW5jbHVkZSAiQjNJbnNl cnRpb25TZXRJbmxpbmVzLmgiCiAjaW5jbHVkZSAiQjNNZW1vcnlWYWx1ZS5oIgogI2luY2x1ZGUg IkIzUGhhc2VTY29wZS5oIgorI2luY2x1ZGUgIkIzUGhpQ2hpbGRyZW4uaCIKICNpbmNsdWRlICJC M1Byb2NlZHVyZUlubGluZXMuaCIKICNpbmNsdWRlICJCM1Vwc2lsb25WYWx1ZS5oIgogI2luY2x1 ZGUgIkIzVXNlQ291bnRzLmgiCkBAIC0xMjIsNiArMTIzLDcgQEAgcHVibGljOgogICAgICAgICAg ICAgfQogCiAgICAgICAgICAgICBraWxsRGVhZENvZGUoKTsKKyAgICAgICAgICAgIHNpbXBsaWZ5 U1NBKCk7CiAgICAgICAgICAgICAKICAgICAgICAgICAgIHJlc3VsdCB8PSBtX2NoYW5nZWQ7CiAg ICAgICAgIH0gd2hpbGUgKG1fY2hhbmdlZCk7CkBAIC0xMjEwLDYgKzEyMTIsNjcgQEAgcHJpdmF0 ZToKICAgICAgICAgfQogICAgIH0KIAorICAgIHZvaWQgc2ltcGxpZnlTU0EoKQorICAgIHsKKyAg ICAgICAgLy8gVGhpcyBydW5zIEF5Y29jayBhbmQgSG9yc3Bvb2wncyBhbGdvcml0aG0gb24gb3Vy IFBoaSBmdW5jdGlvbnMgWzFdLiBGb3IgbW9zdCBDRkcgcGF0dGVybnMsCisgICAgICAgIC8vIHRo aXMgY2FuIHRha2UgYSBzdWJvcHRpbWFsIGFycmFuZ2VtZW50IG9mIFBoaSBmdW5jdGlvbnMgYW5k IG1ha2UgaXQgb3B0aW1hbCwgYXMgaWYgeW91IGhhZAorICAgICAgICAvLyBydW4gQ3l0cm9uLCBG ZXJyYW50ZSwgUm9zZW4sIFdlZ21hbiwgYW5kIFphZGVjay4gSXQncyBvbmx5IHN1Ym9wdGltYWwg Zm9yIGlycmVkdWNpYmxlCisgICAgICAgIC8vIENGR3MuIEluIHByYWN0aWNlLCB0aGF0IGRvZXNu J3QgbWF0dGVyLCBzaW5jZSB3ZSBleHBlY3QgY2xpZW50cyBvZiBCMyB0byBydW4gdGhlaXIgb3du IFNTQQorICAgICAgICAvLyBjb252ZXJzaW9uIGJlZm9yZSBsb3dlcmluZyB0byBCMywgYW5kIGlu IHRoZSBjYXNlIG9mIHRoZSBERkcsIHRoYXQgY29udmVyc2lvbiB1c2VzIEN5dHJvbgorICAgICAg ICAvLyBldCBhbC4gSW4gdGhhdCBjb250ZXh0LCB0aGlzIGFsZ29yaXRobSBpcyBpbnRlbmRlZCB0 byBzaW1wbGlmeSBQaGkgZnVuY3Rpb25zIHRoYXQgd2VyZQorICAgICAgICAvLyBtYWRlIHJlZHVu ZGFudCBieSBwcmlvciBDRkcgc2ltcGxpZmljYXRpb24uIEJ1dCBhY2NvcmRpbmcgdG8gQXljb2Nr IGFuZCBIb3JzcG9vbCdzIHBhcGVyLAorICAgICAgICAvLyB0aGlzIGFsZ29yaXRobSBpcyBnb29k IGVub3VnaCB0aGF0IGEgQjMgY2xpZW50IGNvdWxkIGp1c3QgZ2l2ZSB1cyBtYXhpbWFsIFBoaSdz IChpLmUuIFBoaQorICAgICAgICAvLyBmb3IgZWFjaCB2YXJpYWJsZSBhdCBlYWNoIGJhc2ljIGJs b2NrKSBhbmQgd2Ugd2lsbCBtYWtlIHRoZW0gb3B0aW1hbC4KKyAgICAgICAgLy8gWzFdIGh0dHA6 Ly9wYWdlcy5jcHNjLnVjYWxnYXJ5LmNhL35heWNvY2svcGFwZXJzL3NzYS5wcworCisgICAgICAg IC8vIEF5Y29jayBhbmQgSG9yc3Bvb2wgcHJlc2NyaWJlIHR3byBydWxlcyB0aGF0IGFyZSB0byBi ZSBydW4gdG8gZml4cG9pbnQ6CisgICAgICAgIC8vCisgICAgICAgIC8vIDEpIElmIGFsbCBvZiB0 aGUgUGhpJ3MgY2hpbGRyZW4gYXJlIHRoZSBzYW1lIChpLmUuIGl0J3Mgb25lIGNoaWxkIHJlZmVy ZW5jZWQgZnJvbSBvbmUgb3IKKyAgICAgICAgLy8gICAgbW9yZSBVcHNpbG9ucyksIHRoZW4gcmVw bGFjZSBhbGwgdXNlcyBvZiB0aGUgUGhpIHdpdGggdGhlIG9uZSBjaGlsZC4KKyAgICAgICAgLy8K KyAgICAgICAgLy8gMikgSWYgYWxsIG9mIHRoZSBQaGkncyBjaGlsZHJlbiBhcmUgZWl0aGVyIHRo ZSBQaGkgaXRzZWxmIG9yIGV4YWN0bHkgb25lIG90aGVyIGNoaWxkLCB0aGVuCisgICAgICAgIC8v ICAgIHJlcGxhY2UgYWxsIHVzZXMgb2YgdGhlIFBoaSB3aXRoIHRoZSBvbmUgb3RoZXIgY2hpbGQu CisgICAgICAgIC8vCisgICAgICAgIC8vIFJ1bGUgKDIpIHN1YnN1bWVzIHJ1bGUgKDEpLCBzbyB3 ZSBjYW4ganVzdCBydW4gKDIpLiBXZSBvbmx5IHJ1biBvbmUgZml4cG9pbnQgaXRlcmF0aW9uCisg ICAgICAgIC8vIGhlcmUuIFRoaXMgcHJlbWlzZSBpcyB0aGF0IGluIGNvbW1vbiBjYXNlcywgdGhp cyB3aWxsIG9ubHkgZmluZCBvcHRpbWl6YXRpb24gb3Bwb3J0dW5pdGllcworICAgICAgICAvLyBh cyBhIHJlc3VsdCBvZiBDRkcgc2ltcGxpZmljYXRpb24gYW5kIHVzdWFsbHkgQ0ZHIHNpbXBsaWZp Y2F0aW9uIHdpbGwgb25seSBkbyBvbmUgcm91bmQKKyAgICAgICAgLy8gb2YgYmxvY2sgbWVyZ2lu ZyBwZXIgUmVkdWNlU3RyZW5ndGggZml4cG9pbnQgaXRlcmF0aW9uLCBzbyBpdCdzIE9LIGZvciB0 aGlzIHRvIG9ubHkgZG8gb25lCisgICAgICAgIC8vIHJvdW5kIG9mIFBoaSBtZXJnaW5nIC0gc2lu Y2UgUGhpcyBhcmUgdGhlIHZhbHVlIGFuYWxvZ3VlIG9mIGJsb2Nrcy4KKworICAgICAgICBQaGlD aGlsZHJlbiBwaGlDaGlsZHJlbihtX3Byb2MpOworCisgICAgICAgIGZvciAoVmFsdWUqIHBoaSA6 IHBoaUNoaWxkcmVuLnBoaXMoKSkgeworICAgICAgICAgICAgVmFsdWUqIG90aGVyQ2hpbGQgPSBu dWxscHRyOworICAgICAgICAgICAgYm9vbCBvayA9IHRydWU7CisgICAgICAgICAgICBmb3IgKFZh bHVlKiBjaGlsZCA6IHBoaUNoaWxkcmVuW3BoaV0udmFsdWVzKCkpIHsKKyAgICAgICAgICAgICAg ICBpZiAoY2hpbGQgPT0gcGhpKQorICAgICAgICAgICAgICAgICAgICBjb250aW51ZTsKKyAgICAg ICAgICAgICAgICBpZiAoY2hpbGQgPT0gb3RoZXJDaGlsZCkKKyAgICAgICAgICAgICAgICAgICAg Y29udGludWU7CisgICAgICAgICAgICAgICAgaWYgKG90aGVyQ2hpbGQgPT0gbnVsbHB0cikgewor ICAgICAgICAgICAgICAgICAgICBvdGhlckNoaWxkID0gY2hpbGQ7CisgICAgICAgICAgICAgICAg ICAgIGNvbnRpbnVlOworICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAgICAgICBvayA9IGZh bHNlOworICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgfQorICAgICAgICAgICAg aWYgKCFvaykKKyAgICAgICAgICAgICAgICBjb250aW51ZTsKKyAgICAgICAgICAgIGlmICghb3Ro ZXJDaGlsZCkgeworICAgICAgICAgICAgICAgIC8vIFdvdywgdGhpcyB3b3VsZCBiZSBzdXBlciB3 ZWlyZC4gSXQgcHJvYmFibHkgd29uJ3QgaGFwcGVuLCBleGNlcHQgdGhhdCB0aGluZ3MgY291bGQK KyAgICAgICAgICAgICAgICAvLyBnZXQgd2VpcmQgYXMgYSBjb25zZXF1ZW5jZSBvZiBzdGVwd2lz ZSBzaW1wbGlmaWNhdGlvbnMgaW4gdGhlIHN0cmVuZ3RoIHJlZHVjdGlvbgorICAgICAgICAgICAg ICAgIC8vIGZpeHBvaW50LgorICAgICAgICAgICAgICAgIGNvbnRpbnVlOworICAgICAgICAgICAg fQorICAgICAgICAgICAgCisgICAgICAgICAgICAvLyBUdXJuIHRoZSBQaGkgaW50byBhbiBJZGVu dGl0eSBhbmQgdHVybiB0aGUgVXBzaWxvbnMgaW50byBOb3BzLgorICAgICAgICAgICAgbV9jaGFu Z2VkID0gdHJ1ZTsKKyAgICAgICAgICAgIGZvciAoVmFsdWUqIHVwc2lsb24gOiBwaGlDaGlsZHJl bltwaGldKQorICAgICAgICAgICAgICAgIHVwc2lsb24tPnJlcGxhY2VXaXRoTm9wKCk7CisgICAg ICAgICAgICBwaGktPnJlcGxhY2VXaXRoSWRlbnRpdHkob3RoZXJDaGlsZCk7CisgICAgICAgIH0K KyAgICB9CisKICAgICBQcm9jZWR1cmUmIG1fcHJvYzsKICAgICBJbnNlcnRpb25TZXQgbV9pbnNl cnRpb25TZXQ7CiAgICAgQmFzaWNCbG9jayogbV9ibG9jazsK