154743 2016-02-26 12:46:22 -0800 Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup 2016-02-27 15:04:12 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 keith_miller keith_miller commit-queue mark.lam msaboff saam oldest_to_newest 1168629 0 keith_miller 2016-02-26 12:46:22 -0800 Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup 1168632 1 272358 keith_miller 2016-02-26 12:47:34 -0800 Created attachment 272358 Patch 1168634 2 272358 mark.lam 2016-02-26 12:52:39 -0800 Comment on attachment 272358 Patch r=me 1168659 3 272358 commit-queue 2016-02-26 13:43:04 -0800 Comment on attachment 272358 Patch Clearing flags on attachment: 272358 Committed r197196: <http://trac.webkit.org/changeset/197196> 1168660 4 commit-queue 2016-02-26 13:43:07 -0800 All reviewed patches have been landed. Closing bug. 1168681 5 272358 ggaren 2016-02-26 14:12:45 -0800 Comment on attachment 272358 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272358&action=review > Source/JavaScriptCore/ChangeLog:3 > + Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup Why? 1168702 6 keith_miller 2016-02-26 14:37:44 -0800 (In reply to comment #5) > Comment on attachment 272358 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272358&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup > > Why? When compiling a instanceof b we sometimes don't end up finding that b is a constant until after fixup. If we attempt to fold the OverridesHasInstance in fixup we won't end up removing the check when we otherwise could have. 1168906 7 272358 fpizlo 2016-02-27 15:04:12 -0800 Comment on attachment 272358 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272358&action=review > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:573 > + case OverridesHasInstance: { > + if (!node->child2().node()->isCellConstant()) > + break; > + > + if (node->child2().node()->asCell() != m_graph.globalObjectFor(node->origin.semantic)->functionProtoHasInstanceSymbolFunction()) { > + m_graph.convertToConstant(node, jsBoolean(true)); > + changed = true; > + > + } else if (!m_graph.hasExitSite(node->origin.semantic, BadTypeInfoFlags)) { > + // We optimistically assume that we will not see a function that has a custom instanceof operation as they should be rare. > + m_insertionSet.insertNode(indexInBlock, SpecNone, CheckTypeInfoFlags, node->origin, OpInfo(ImplementsDefaultHasInstance), Edge(node->child1().node(), CellUse)); > + m_graph.convertToConstant(node, jsBoolean(false)); > + changed = true; > + } > + > + break; > + } > + This is not how we write constant folding rules. A constant folding rule *must* occur in DFG::AbstractInterpreterInlines if it occurs in ConstantFoldingPhase. Otherwise, the constant folding rule will have the following silly shortcomings: - It will only work in basic blocks that also incidentally did other constant folding, since you need a rule in AbstractInterpreterInlines to tell the ConstantFoldingPhase to do things in a block. - It will trigger inductive constant folding beyond the current basic block. Since AbstractInterpterInlines thinks that this node returns any boolean, other basic blocks that use this value will not know to constant-fold their uses. A simple rule to follow is: if someone writes a patch that touches ConstantFoldingPhase but not AbstractInterpreterInlines, then their patch is pretty much guaranteed to be wrong. 272358 2016-02-26 12:47:34 -0800 2016-02-26 13:43:04 -0800 Patch bug-154743-20160226124717.patch text/plain 3816 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMTk2ODI3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBm NmU1NTFkMzllMzJmMDkzNTBhMzgzYTJmOWJmNzU1MmFlNTNjOWM0Li5mMDhmZjdjNTkwNjM4NDA0 OWViMjZhZjFlMGM4MjU2MjU2MWVhMjcyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAxNi0wMi0yNiAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBGb2xkaW5nIG9mIE92ZXJyaWRlc0hhc0luc3RhbmNlIERGRyBub2Rl cyBzaG91ZCBoYXBwZW4gaW4gY29uc3RhbnQgZm9sZGluZyBub3QgZml4dXAKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NDc0MworCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZGZnL0RGR0NvbnN0YW50Rm9s ZGluZ1BoYXNlLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpDb25zdGFudEZvbGRpbmdQaGFzZTo6 Zm9sZENvbnN0YW50cyk6CisgICAgICAgICogZGZnL0RGR0ZpeHVwUGhhc2UuY3BwOgorICAgICAg ICAoSlNDOjpERkc6OkZpeHVwUGhhc2U6OmZpeHVwTm9kZSk6CisKIDIwMTYtMDItMTkgIENzYWJh IE9zenRyb2dvbsOhYyAgPG9zc3lAd2Via2l0Lm9yZz4KIAogICAgICAgICBSZW1vdmUgbW9yZSBM TFZNIHJlbGF0ZWQgZGVhZCBjb2RlIGFmdGVyIHIxOTY3MjkKZGlmZiAtLWdpdCBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9kZmcvREZHQ29uc3RhbnRGb2xkaW5nUGhhc2UuY3BwIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL2RmZy9ERkdDb25zdGFudEZvbGRpbmdQaGFzZS5jcHAKaW5kZXggZTViZThl ZmI1NTZkMTdmMzA0Y2EyZmUxNWJlZTNhOWY2N2ZmMzBiZS4uZjUzMjA5YjdkYTdhODkxODcxNjBj MTQ4ZDg3NGY3ZDAwNzcxMjI1YyAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Rm Zy9ERkdDb25zdGFudEZvbGRpbmdQaGFzZS5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L2RmZy9ERkdDb25zdGFudEZvbGRpbmdQaGFzZS5jcHAKQEAgLTU1Myw2ICs1NTMsMjQgQEAgcHJp dmF0ZToKICAgICAgICAgICAgICAgICBicmVhazsKICAgICAgICAgICAgIH0KIAorICAgICAgICAg ICAgY2FzZSBPdmVycmlkZXNIYXNJbnN0YW5jZTogeworICAgICAgICAgICAgICAgIGlmICghbm9k ZS0+Y2hpbGQyKCkubm9kZSgpLT5pc0NlbGxDb25zdGFudCgpKQorICAgICAgICAgICAgICAgICAg ICBicmVhazsKKworICAgICAgICAgICAgICAgIGlmIChub2RlLT5jaGlsZDIoKS5ub2RlKCktPmFz Q2VsbCgpICE9IG1fZ3JhcGguZ2xvYmFsT2JqZWN0Rm9yKG5vZGUtPm9yaWdpbi5zZW1hbnRpYykt PmZ1bmN0aW9uUHJvdG9IYXNJbnN0YW5jZVN5bWJvbEZ1bmN0aW9uKCkpIHsKKyAgICAgICAgICAg ICAgICAgICAgbV9ncmFwaC5jb252ZXJ0VG9Db25zdGFudChub2RlLCBqc0Jvb2xlYW4odHJ1ZSkp OworICAgICAgICAgICAgICAgICAgICBjaGFuZ2VkID0gdHJ1ZTsKKworICAgICAgICAgICAgICAg IH0gZWxzZSBpZiAoIW1fZ3JhcGguaGFzRXhpdFNpdGUobm9kZS0+b3JpZ2luLnNlbWFudGljLCBC YWRUeXBlSW5mb0ZsYWdzKSkgeworICAgICAgICAgICAgICAgICAgICAvLyBXZSBvcHRpbWlzdGlj YWxseSBhc3N1bWUgdGhhdCB3ZSB3aWxsIG5vdCBzZWUgYSBmdW5jdGlvbiB0aGF0IGhhcyBhIGN1 c3RvbSBpbnN0YW5jZW9mIG9wZXJhdGlvbiBhcyB0aGV5IHNob3VsZCBiZSByYXJlLgorICAgICAg ICAgICAgICAgICAgICBtX2luc2VydGlvblNldC5pbnNlcnROb2RlKGluZGV4SW5CbG9jaywgU3Bl Y05vbmUsIENoZWNrVHlwZUluZm9GbGFncywgbm9kZS0+b3JpZ2luLCBPcEluZm8oSW1wbGVtZW50 c0RlZmF1bHRIYXNJbnN0YW5jZSksIEVkZ2Uobm9kZS0+Y2hpbGQxKCkubm9kZSgpLCBDZWxsVXNl KSk7CisgICAgICAgICAgICAgICAgICAgIG1fZ3JhcGguY29udmVydFRvQ29uc3RhbnQobm9kZSwg anNCb29sZWFuKGZhbHNlKSk7CisgICAgICAgICAgICAgICAgICAgIGNoYW5nZWQgPSB0cnVlOwor ICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAgICAgICAKKyAgICAgICAgICAgICAgICBicmVh azsKKyAgICAgICAgICAgIH0KKwogICAgICAgICAgICAgY2FzZSBDaGVjazogewogICAgICAgICAg ICAgICAgIGFscmVhZHlIYW5kbGVkID0gdHJ1ZTsKICAgICAgICAgICAgICAgICBtX2ludGVycHJl dGVyLmV4ZWN1dGUoaW5kZXhJbkJsb2NrKTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9kZmcvREZHRml4dXBQaGFzZS5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RG R0ZpeHVwUGhhc2UuY3BwCmluZGV4IDZhNGViMTNhZWQzMmJiOTdlOGM0ZGE4OTcyODc2MDg0MmM2 MDljYjkuLjIyNjVjMTAwZThmZGU3MTg4MmMxMTFhMmVhYWMzZmYwNDBiYzc5NmIgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHRml4dXBQaGFzZS5jcHAKKysrIGIvU291 cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdGaXh1cFBoYXNlLmNwcApAQCAtMTA5MywyNiArMTA5 Myw3IEBAIHByaXZhdGU6CiAgICAgICAgICAgICBicmVhazsKICAgICAgICAgfQogCi0gICAgICAg IGNhc2UgT3ZlcnJpZGVzSGFzSW5zdGFuY2U6IHsKLSAgICAgICAgICAgIGlmIChub2RlLT5jaGls ZDIoKS5ub2RlKCktPmlzQ2VsbENvbnN0YW50KCkpIHsKLSAgICAgICAgICAgICAgICBpZiAobm9k ZS0+Y2hpbGQyKCkubm9kZSgpLT5hc0NlbGwoKSAhPSBtX2dyYXBoLmdsb2JhbE9iamVjdEZvcihu b2RlLT5vcmlnaW4uc2VtYW50aWMpLT5mdW5jdGlvblByb3RvSGFzSW5zdGFuY2VTeW1ib2xGdW5j dGlvbigpKSB7Ci0KLSAgICAgICAgICAgICAgICAgICAgbV9ncmFwaC5jb252ZXJ0VG9Db25zdGFu dChub2RlLCBqc0Jvb2xlYW4odHJ1ZSkpOwotICAgICAgICAgICAgICAgICAgICBicmVhazsKLSAg ICAgICAgICAgICAgICB9Ci0KLSAgICAgICAgICAgICAgICBpZiAoIW1fZ3JhcGguaGFzRXhpdFNp dGUobm9kZS0+b3JpZ2luLnNlbWFudGljLCBCYWRUeXBlSW5mb0ZsYWdzKSkgewotICAgICAgICAg ICAgICAgICAgICAvLyBIZXJlIHdlIG9wdGltaXN0aWNhbGx5IGFzc3VtZSB0aGF0IHdlIHdpbGwg bm90IHNlZSBhbiBib3VuZC9DLUFQSSBmdW5jdGlvbiBoZXJlLgotICAgICAgICAgICAgICAgICAg ICBtX2luc2VydGlvblNldC5pbnNlcnROb2RlKG1faW5kZXhJbkJsb2NrLCBTcGVjTm9uZSwgQ2hl Y2tUeXBlSW5mb0ZsYWdzLCBub2RlLT5vcmlnaW4sIE9wSW5mbyhJbXBsZW1lbnRzRGVmYXVsdEhh c0luc3RhbmNlKSwgRWRnZShub2RlLT5jaGlsZDEoKS5ub2RlKCksIENlbGxVc2UpKTsKLSAgICAg ICAgICAgICAgICAgICAgbV9ncmFwaC5jb252ZXJ0VG9Db25zdGFudChub2RlLCBqc0Jvb2xlYW4o ZmFsc2UpKTsKLSAgICAgICAgICAgICAgICAgICAgYnJlYWs7Ci0gICAgICAgICAgICAgICAgfQot ICAgICAgICAgICAgfQotCi0gICAgICAgICAgICBmaXhFZGdlPENlbGxVc2U+KG5vZGUtPmNoaWxk MSgpKTsKLSAgICAgICAgICAgIGJyZWFrOwotICAgICAgICB9Ci0gICAgICAgICAgICAKKyAgICAg ICAgY2FzZSBPdmVycmlkZXNIYXNJbnN0YW5jZToKICAgICAgICAgY2FzZSBDaGVja1N0cnVjdHVy ZToKICAgICAgICAgY2FzZSBDaGVja0NlbGw6CiAgICAgICAgIGNhc2UgQ3JlYXRlVGhpczoK