118957 2013-07-21 15:38:54 -0700 fourthTier: It should be easy to figure out which blocks nodes belong to 2013-07-21 20:57:03 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 118749 1 fpizlo fpizlo barraclough ggaren mark.lam mhahnenberg msaboff oliver sam oldest_to_newest 910552 0 fpizlo 2013-07-21 15:38:54 -0700 Patch forthcoming. 910554 1 207217 fpizlo 2013-07-21 16:10:37 -0700 Created attachment 207217 the patch 910571 2 207217 sam 2013-07-21 18:17:11 -0700 Comment on attachment 207217 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=207217&action=review > Source/JavaScriptCore/dfg/DFGNode.h:1404 > union { > Node* replacement; > + BasicBlock* owner; > } misc; Can you add clarification about what ensures that a node only has either a replacement or an owner. 910580 3 fpizlo 2013-07-21 20:40:54 -0700 (In reply to comment #2) > (From update of attachment 207217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207217&action=review > > > Source/JavaScriptCore/dfg/DFGNode.h:1404 > > union { > > Node* replacement; > > + BasicBlock* owner; > > } misc; > > Can you add clarification about what ensures that a node only has either a replacement or an owner. It's not like that. These fields only become valid if you initialize them. You have to make up your mind, when you initialize them, whether your phase needs to use replacement, or needs to know the node-to-block mapping. Most phases need neither of these pieces of information. Only three phases use replacements, and the only phases that will use owners are LICM and possibly GVN. 910581 4 207222 fpizlo 2013-07-21 20:43:37 -0700 Created attachment 207222 the patch 910582 5 207217 sam 2013-07-21 20:44:27 -0700 Comment on attachment 207217 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=207217&action=review >>> Source/JavaScriptCore/dfg/DFGNode.h:1404 >>> } misc; >> >> Can you add clarification about what ensures that a node only has either a replacement or an owner. > > It's not like that. These fields only become valid if you initialize them. You have to make up your mind, when you initialize them, whether your phase needs to use replacement, or needs to know the node-to-block mapping. Most phases need neither of these pieces of information. Only three phases use replacements, and the only phases that will use owners are LICM and possibly GVN. Ok. You probably want to add some indication of that (or an assertion if possible), as it wasn't clear. 910583 6 sam 2013-07-21 20:45:12 -0700 (In reply to comment #5) > (From update of attachment 207217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207217&action=review > > >>> Source/JavaScriptCore/dfg/DFGNode.h:1404 > >>> } misc; > >> > >> Can you add clarification about what ensures that a node only has either a replacement or an owner. > > > > It's not like that. These fields only become valid if you initialize them. You have to make up your mind, when you initialize them, whether your phase needs to use replacement, or needs to know the node-to-block mapping. Most phases need neither of these pieces of information. Only three phases use replacements, and the only phases that will use owners are LICM and possibly GVN. > > Ok. You probably want to add some indication of that (or an assertion if possible), as it wasn't clear. Moments later, you updated it! r=me. 910584 7 fpizlo 2013-07-21 20:48:40 -0700 (In reply to comment #5) > (From update of attachment 207217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207217&action=review > > >>> Source/JavaScriptCore/dfg/DFGNode.h:1404 > >>> } misc; > >> > >> Can you add clarification about what ensures that a node only has either a replacement or an owner. > > > > It's not like that. These fields only become valid if you initialize them. You have to make up your mind, when you initialize them, whether your phase needs to use replacement, or needs to know the node-to-block mapping. Most phases need neither of these pieces of information. Only three phases use replacements, and the only phases that will use owners are LICM and possibly GVN. > > Ok. You probably want to add some indication of that (or an assertion if possible), as it wasn't clear. I don't think an assertion is either possible or profitable - we don't have any kind of natural way of checking if a pointer points to a Node or a BasicBlock. Also, each phase first initializes 'misc', and then uses it, often in a very obvious way. Like, you'll literally see one loop that initializes them and another loop that uses them. 910585 8 fpizlo 2013-07-21 20:57:03 -0700 Landed in http://trac.webkit.org/changeset/152958 207217 2013-07-21 16:10:37 -0700 2013-07-21 20:44:27 -0700 the patch blah.patch text/plain 2459 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTUyOTUzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDEzLTA3LTIxICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg Zm91cnRoVGllcjogSXQgc2hvdWxkIGJlIGVhc3kgdG8gZmlndXJlIG91dCB3aGljaCBibG9ja3Mg bm9kZXMgYmVsb25nIHRvCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xMTg5NTcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIGRmZy9ERkdHcmFwaC5jcHA6CisgICAgICAgIChERkcpOgorICAgICAgICAoSlND OjpERkc6OkdyYXBoOjppbml0aWFsaXplTm9kZU93bmVycyk6CisgICAgICAgICogZGZnL0RGR0dy YXBoLmg6CisgICAgICAgIChHcmFwaCk6CisgICAgICAgICogZGZnL0RGR05vZGUuaDoKKwogMjAx My0wNy0yMCAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIGZvdXJ0 aFRpZXI6IGVhY2ggREZHIG5vZGUgdGhhdCByZWxpZXMgb24gb3RoZXIgbm9kZXMgdG8gZG8gdGhl aXIgdHlwZSBjaGVja3Mgc2hvdWxkIGJlIGFibGUgdG8gdGVsbCB5b3UgaWYgdGhvc2UgdHlwZSBj aGVja3MgaGFwcGVuZWQKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGgu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGguY3Bw CShyZXZpc2lvbiAxNTI5NTMpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0dyYXBo LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTQ2LDYgKzU0NiwxOSBAQCB2b2lkIEdyYXBoOjpjbGVh clJlcGxhY2VtZW50cygpCiAgICAgICAgICAgICBibG9jay0+YXQobm9kZUluZGV4KS0+bWlzYy5y ZXBsYWNlbWVudCA9IDA7CiAgICAgfQogfQorCit2b2lkIEdyYXBoOjppbml0aWFsaXplTm9kZU93 bmVycygpCit7CisgICAgZm9yIChCbG9ja0luZGV4IGJsb2NrSW5kZXggPSBudW1CbG9ja3MoKTsg YmxvY2tJbmRleC0tOykgeworICAgICAgICBCYXNpY0Jsb2NrKiBibG9jayA9IG1fYmxvY2tzW2Js b2NrSW5kZXhdLmdldCgpOworICAgICAgICBpZiAoIWJsb2NrKQorICAgICAgICAgICAgY29udGlu dWU7CisgICAgICAgIGZvciAodW5zaWduZWQgcGhpSW5kZXggPSBibG9jay0+cGhpcy5zaXplKCk7 IHBoaUluZGV4LS07KQorICAgICAgICAgICAgYmxvY2stPnBoaXNbcGhpSW5kZXhdLT5taXNjLm93 bmVyID0gYmxvY2s7CisgICAgICAgIGZvciAodW5zaWduZWQgbm9kZUluZGV4ID0gYmxvY2stPnNp emUoKTsgbm9kZUluZGV4LS07KQorICAgICAgICAgICAgYmxvY2stPmF0KG5vZGVJbmRleCktPm1p c2Mub3duZXIgPSBibG9jazsKKyAgICB9Cit9CiAgICAgCiB9IH0gLy8gbmFtZXNwYWNlIEpTQzo6 REZHCiAKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGguaAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0dyYXBoLmgJKHJldmlzaW9uIDE1 Mjk1MykKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGguaAkod29ya2luZyBj b3B5KQpAQCAtNjQ3LDYgKzY0Nyw3IEBAIHB1YmxpYzoKICAgICB2b2lkIGludmFsaWRhdGVDRkco KTsKICAgICAKICAgICB2b2lkIGNsZWFyUmVwbGFjZW1lbnRzKCk7CisgICAgdm9pZCBpbml0aWFs aXplTm9kZU93bmVycygpOwogICAgIAogICAgIHZvaWQgZ2V0QmxvY2tzSW5EZXB0aEZpcnN0T3Jk ZXIoVmVjdG9yPEJhc2ljQmxvY2sqPiYgcmVzdWx0KTsKICAgICAKSW5kZXg6IFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9kZmcvREZHTm9kZS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9kZmcvREZHTm9kZS5oCShyZXZpc2lvbiAxNTI5NTMpCisrKyBTb3VyY2UvSmF2YVNjcmlw dENvcmUvZGZnL0RGR05vZGUuaAkod29ya2luZyBjb3B5KQpAQCAtMTQwMCw2ICsxNDAwLDcgQEAg cHVibGljOgogICAgIEFic3RyYWN0VmFsdWUgdmFsdWU7CiAgICAgdW5pb24gewogICAgICAgICBO b2RlKiByZXBsYWNlbWVudDsKKyAgICAgICAgQmFzaWNCbG9jayogb3duZXI7CiAgICAgfSBtaXNj OwogfTsKIAo= 207222 2013-07-21 20:43:37 -0700 2013-07-21 20:44:41 -0700 the patch blah.patch text/plain 3260 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTUyOTUzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDEzLTA3LTIxICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg Zm91cnRoVGllcjogSXQgc2hvdWxkIGJlIGVhc3kgdG8gZmlndXJlIG91dCB3aGljaCBibG9ja3Mg bm9kZXMgYmVsb25nIHRvCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xMTg5NTcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIGRmZy9ERkdHcmFwaC5jcHA6CisgICAgICAgIChERkcpOgorICAgICAgICAoSlND OjpERkc6OkdyYXBoOjppbml0aWFsaXplTm9kZU93bmVycyk6CisgICAgICAgICogZGZnL0RGR0dy YXBoLmg6CisgICAgICAgIChHcmFwaCk6CisgICAgICAgICogZGZnL0RGR05vZGUuaDoKKwogMjAx My0wNy0yMCAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIGZvdXJ0 aFRpZXI6IGVhY2ggREZHIG5vZGUgdGhhdCByZWxpZXMgb24gb3RoZXIgbm9kZXMgdG8gZG8gdGhl aXIgdHlwZSBjaGVja3Mgc2hvdWxkIGJlIGFibGUgdG8gdGVsbCB5b3UgaWYgdGhvc2UgdHlwZSBj aGVja3MgaGFwcGVuZWQKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGgu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGguY3Bw CShyZXZpc2lvbiAxNTI5NTMpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0dyYXBo LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTQ2LDYgKzU0NiwxOSBAQCB2b2lkIEdyYXBoOjpjbGVh clJlcGxhY2VtZW50cygpCiAgICAgICAgICAgICBibG9jay0+YXQobm9kZUluZGV4KS0+bWlzYy5y ZXBsYWNlbWVudCA9IDA7CiAgICAgfQogfQorCit2b2lkIEdyYXBoOjppbml0aWFsaXplTm9kZU93 bmVycygpCit7CisgICAgZm9yIChCbG9ja0luZGV4IGJsb2NrSW5kZXggPSBudW1CbG9ja3MoKTsg YmxvY2tJbmRleC0tOykgeworICAgICAgICBCYXNpY0Jsb2NrKiBibG9jayA9IG1fYmxvY2tzW2Js b2NrSW5kZXhdLmdldCgpOworICAgICAgICBpZiAoIWJsb2NrKQorICAgICAgICAgICAgY29udGlu dWU7CisgICAgICAgIGZvciAodW5zaWduZWQgcGhpSW5kZXggPSBibG9jay0+cGhpcy5zaXplKCk7 IHBoaUluZGV4LS07KQorICAgICAgICAgICAgYmxvY2stPnBoaXNbcGhpSW5kZXhdLT5taXNjLm93 bmVyID0gYmxvY2s7CisgICAgICAgIGZvciAodW5zaWduZWQgbm9kZUluZGV4ID0gYmxvY2stPnNp emUoKTsgbm9kZUluZGV4LS07KQorICAgICAgICAgICAgYmxvY2stPmF0KG5vZGVJbmRleCktPm1p c2Mub3duZXIgPSBibG9jazsKKyAgICB9Cit9CiAgICAgCiB9IH0gLy8gbmFtZXNwYWNlIEpTQzo6 REZHCiAKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGguaAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0dyYXBoLmgJKHJldmlzaW9uIDE1 Mjk1MykKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHR3JhcGguaAkod29ya2luZyBj b3B5KQpAQCAtNjQ3LDYgKzY0Nyw3IEBAIHB1YmxpYzoKICAgICB2b2lkIGludmFsaWRhdGVDRkco KTsKICAgICAKICAgICB2b2lkIGNsZWFyUmVwbGFjZW1lbnRzKCk7CisgICAgdm9pZCBpbml0aWFs aXplTm9kZU93bmVycygpOwogICAgIAogICAgIHZvaWQgZ2V0QmxvY2tzSW5EZXB0aEZpcnN0T3Jk ZXIoVmVjdG9yPEJhc2ljQmxvY2sqPiYgcmVzdWx0KTsKICAgICAKSW5kZXg6IFNvdXJjZS9KYXZh U2NyaXB0Q29yZS9kZmcvREZHTm9kZS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9kZmcvREZHTm9kZS5oCShyZXZpc2lvbiAxNTI5NTMpCisrKyBTb3VyY2UvSmF2YVNjcmlw dENvcmUvZGZnL0RGR05vZGUuaAkod29ya2luZyBjb3B5KQpAQCAtMTM5OCw4ICsxMzk4LDIwIEBA IHByaXZhdGU6CiBwdWJsaWM6CiAgICAgLy8gRmllbGRzIHVzZWQgYnkgdmFyaW91cyBhbmFseXNl cy4KICAgICBBYnN0cmFjdFZhbHVlIHZhbHVlOworICAgIAorICAgIC8vIE1pc2NlbGxhbmVvdXMg ZGF0YSB0aGF0IGlzIHVzdWFsbHkgbWVhbmluZ2xlc3MsIGJ1dCBjYW4gaG9sZCBzb21lIGFuYWx5 c2lzIHJlc3VsdHMKKyAgICAvLyBpZiB5b3UgYXNrIHJpZ2h0LiBGb3IgZXhhbXBsZSwgaWYgeW91 IGRvIEdyYXBoOjppbml0aWFsaXplTm9kZU93bmVycygpLCBtaXNjLm93bmVyCisgICAgLy8gd2ls bCB0ZWxsIHlvdSB3aGljaCBiYXNpYyBibG9jayBhIG5vZGUgYmVsb25ncyB0by4gWW91IGNhbm5v dCByZWx5IG9uIHRoaXMgcGVyc2lzdGluZworICAgIC8vIGFjcm9zcyB0cmFuc2Zvcm1hdGlvbnMg dW5sZXNzIHlvdSBkbyB0aGUgbWFpbnRlbmFuY2Ugd29yayB5b3Vyc2VsZi4gT3RoZXIgcGhhc2Vz IHVzZQorICAgIC8vIG1pc2MucmVwbGFjZW1lbnQsIGJ1dCB0aGV5IGRvIHNvIG1hbnVhbGx5OiBm aXJzdCB5b3UgZG8gR3JhcGg6OmNsZWFyUmVwbGFjZW1lbnRzKCkKKyAgICAvLyBhbmQgdGhlbiB5 b3Ugc2V0LCBhbmQgdXNlLCByZXBsYWNlbWVudCdzIHlvdXJzZWxmLgorICAgIC8vCisgICAgLy8g Qm90dG9tIGxpbmU6IGRvbid0IHVzZSB0aGVzZSBmaWVsZHMgdW5sZXNzIHlvdSBpbml0aWFsaXpl IHRoZW0geW91cnNlbGYsIG9yIGJ5CisgICAgLy8gY2FsbGluZyBzb21lIGFwcHJvcHJpYXRlIG1l dGhvZHMgdGhhdCBpbml0aWFsaXplIHRoZW0gdGhlIHdheSB5b3Ugd2FudC4gT3RoZXJ3aXNlLAor ICAgIC8vIHRoZXNlIGZpZWxkcyBhcmUgbWVhbmluZ2xlc3MuCiAgICAgdW5pb24gewogICAgICAg ICBOb2RlKiByZXBsYWNlbWVudDsKKyAgICAgICAgQmFzaWNCbG9jayogb3duZXI7CiAgICAgfSBt aXNjOwogfTsKIAo=