140137 2015-01-06 11:51:21 -0800 MultiGetByOffset should be marked NodeMustGenerate 2015-02-02 20:31:06 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 fpizlo fpizlo barraclough ggaren mark.lam mhahnenb mmirman msaboff nrotem oliver saam sam oldest_to_newest 1059094 0 fpizlo 2015-01-06 11:51:21 -0800 Nodes that perform speculations that are necessary to prove that the equivalent bytecode operation is pure must be marked NodeMustGenerate. MultiGetByOffset checks the structure of its input. If the incoming object didn't have that structure, then the equivalent bytecode operation (get_by_id) might do something effectful. But, it's not marked NodeMustGenerate. We should fix that. 1066329 1 245919 fpizlo 2015-02-02 19:48:51 -0800 Created attachment 245919 the patch 1066331 2 245919 msaboff 2015-02-02 19:54:23 -0800 Comment on attachment 245919 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245919&action=review r=me with spelling fixes in ChangeLog. > Source/JavaScriptCore/ChangeLog:10 > + (JSC::DFG::Node::convertToMultiGetByOffset): Assert that we converted from something that alraedy had NodeMustGenerate. *already* > Source/JavaScriptCore/ChangeLog:11 > + * dfg/DFGNodeType.h: We shouldn't DCE a node that does checks and could be effectful in bseline. Making MultiGetBYoffset as NodeMustGenerate prevents DCE. FTL could still DCE the actual loads, but the checks will stay. *baseline* 1066332 3 fpizlo 2015-02-02 19:55:11 -0800 (In reply to comment #2) > Comment on attachment 245919 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245919&action=review > > r=me with spelling fixes in ChangeLog. > > > Source/JavaScriptCore/ChangeLog:10 > > + (JSC::DFG::Node::convertToMultiGetByOffset): Assert that we converted from something that alraedy had NodeMustGenerate. > > *already* > > > Source/JavaScriptCore/ChangeLog:11 > > + * dfg/DFGNodeType.h: We shouldn't DCE a node that does checks and could be effectful in bseline. Making MultiGetBYoffset as NodeMustGenerate prevents DCE. FTL could still DCE the actual loads, but the checks will stay. > > *baseline* Thanks! Fixed both locally. 1066344 4 fpizlo 2015-02-02 20:31:06 -0800 Landed in http://trac.webkit.org/changeset/179536 245919 2015-02-02 19:48:51 -0800 2015-02-02 19:54:23 -0800 the patch blah.patch text/plain 3580 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTc5NTM0KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE1LTAyLTAyICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg TXVsdGlHZXRCeU9mZnNldCBzaG91bGQgYmUgbWFya2VkIE5vZGVNdXN0R2VuZXJhdGUKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0MDEzNworCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZGZnL0RGR05vZGUu aDoKKyAgICAgICAgKEpTQzo6REZHOjpOb2RlOjpjb252ZXJ0VG9HZXRCeU9mZnNldCk6IFdlIHdl cmUgc2xvcHB5IC0gd2Ugc2hvdWxkIGFsc28gY2xlYXIgTm9kZU11c3RHZW5lcmF0ZSBvbmNlIGl0 J3MgYSBHZXRCeU9mZnNldC4KKyAgICAgICAgKEpTQzo6REZHOjpOb2RlOjpjb252ZXJ0VG9NdWx0 aUdldEJ5T2Zmc2V0KTogQXNzZXJ0IHRoYXQgd2UgY29udmVydGVkIGZyb20gc29tZXRoaW5nIHRo YXQgYWxyYWVkeSBoYWQgTm9kZU11c3RHZW5lcmF0ZS4KKyAgICAgICAgKiBkZmcvREZHTm9kZVR5 cGUuaDogV2Ugc2hvdWxkbid0IERDRSBhIG5vZGUgdGhhdCBkb2VzIGNoZWNrcyBhbmQgY291bGQg YmUgZWZmZWN0ZnVsIGluIGJzZWxpbmUuIE1ha2luZyBNdWx0aUdldEJZb2Zmc2V0IGFzIE5vZGVN dXN0R2VuZXJhdGUgcHJldmVudHMgRENFLiBGVEwgY291bGQgc3RpbGwgRENFIHRoZSBhY3R1YWwg bG9hZHMsIGJ1dCB0aGUgY2hlY2tzIHdpbGwgc3RheS4KKyAgICAgICAgKiB0ZXN0cy9zdHJlc3Mv bXVsdGktZ2V0LWJ5LW9mZnNldC1kY2UuanM6IEFkZGVkLiBUaGlzIHByZXZpb3VzbHkgZmFpbGVk IGJlY2F1c2UgdGhlIGdldHRlciB3YXNuJ3QgY2FsbGVkLgorICAgICAgICAoZm9vKToKKwogMjAx NS0wMi0wMiAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIFtGVExd IGlubGluZWQgR2V0TXlBcmd1bWVudEJ5VmFsIHdpdGggbm8gYXJndW1lbnRzIHBhc3NlZCBjYXVz ZXMgaW5zdGFudCBjcmFzaApJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdOb2Rl LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdOb2RlLmgJKHJl dmlzaW9uIDE3OTUzMykKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHTm9kZS5oCSh3 b3JraW5nIGNvcHkpCkBAIC00NzgsNyArNDc4LDcgQEAgc3RydWN0IE5vZGUgewogICAgICAgICBj aGlsZHJlbi5jaGlsZDIoKS5zZXRVc2VLaW5kKEtub3duQ2VsbFVzZSk7CiAgICAgICAgIGNoaWxk cmVuLnNldENoaWxkMShzdG9yYWdlKTsKICAgICAgICAgbV9vcCA9IEdldEJ5T2Zmc2V0OwotICAg ICAgICBtX2ZsYWdzICY9IH5Ob2RlQ2xvYmJlcnNXb3JsZDsKKyAgICAgICAgbV9mbGFncyAmPSB+ KE5vZGVDbG9iYmVyc1dvcmxkIHwgTm9kZU11c3RHZW5lcmF0ZSk7CiAgICAgfQogICAgIAogICAg IHZvaWQgY29udmVydFRvTXVsdGlHZXRCeU9mZnNldChNdWx0aUdldEJ5T2Zmc2V0RGF0YSogZGF0 YSkKQEAgLTQ4OCw2ICs0ODgsNyBAQCBzdHJ1Y3QgTm9kZSB7CiAgICAgICAgIGNoaWxkMSgpLnNl dFVzZUtpbmQoQ2VsbFVzZSk7CiAgICAgICAgIG1fb3AgPSBNdWx0aUdldEJ5T2Zmc2V0OwogICAg ICAgICBtX2ZsYWdzICY9IH5Ob2RlQ2xvYmJlcnNXb3JsZDsKKyAgICAgICAgQVNTRVJUKG1fZmxh Z3MgJiBOb2RlTXVzdEdlbmVyYXRlKTsKICAgICB9CiAgICAgCiAgICAgdm9pZCBjb252ZXJ0VG9Q dXRCeU9mZnNldChTdG9yYWdlQWNjZXNzRGF0YSYgZGF0YSwgRWRnZSBzdG9yYWdlKQpJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdOb2RlVHlwZS5oCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHTm9kZVR5cGUuaAkocmV2aXNpb24gMTc5NTMzKQor KysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdOb2RlVHlwZS5oCSh3b3JraW5nIGNvcHkp CkBAIC0xNzEsNyArMTcxLDcgQEAgbmFtZXNwYWNlIEpTQyB7IG5hbWVzcGFjZSBERkcgewogICAg IG1hY3JvKEdldFNldHRlciwgTm9kZVJlc3VsdEpTKSBcCiAgICAgbWFjcm8oR2V0QnlPZmZzZXQs IE5vZGVSZXN1bHRKUykgXAogICAgIG1hY3JvKEdldEdldHRlclNldHRlckJ5T2Zmc2V0LCBOb2Rl UmVzdWx0SlMpIFwKLSAgICBtYWNybyhNdWx0aUdldEJ5T2Zmc2V0LCBOb2RlUmVzdWx0SlMpIFwK KyAgICBtYWNybyhNdWx0aUdldEJ5T2Zmc2V0LCBOb2RlUmVzdWx0SlMgfCBOb2RlTXVzdEdlbmVy YXRlKSBcCiAgICAgbWFjcm8oUHV0QnlPZmZzZXQsIE5vZGVNdXN0R2VuZXJhdGUpIFwKICAgICBt YWNybyhNdWx0aVB1dEJ5T2Zmc2V0LCBOb2RlTXVzdEdlbmVyYXRlKSBcCiAgICAgbWFjcm8oR2V0 QXJyYXlMZW5ndGgsIE5vZGVSZXN1bHRJbnQzMikgXApJbmRleDogU291cmNlL0phdmFTY3JpcHRD b3JlL3Rlc3RzL3N0cmVzcy9tdWx0aS1nZXQtYnktb2Zmc2V0LWRjZS5qcwo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvdGVzdHMvc3RyZXNzL211bHRpLWdldC1ieS1vZmZzZXQt ZGNlLmpzCShyZXZpc2lvbiAwKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3Rlc3RzL3N0cmVz cy9tdWx0aS1nZXQtYnktb2Zmc2V0LWRjZS5qcwkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDIy IEBACitmdW5jdGlvbiBmb28obykgeworICAgIHZhciB0bXAgPSBvLmY7CisgICAgcmV0dXJuIDQy OworfQorCitub0lubGluZShmb28pOworCit2YXIgYXJyYXkgPSBbe2Y6MX0sIHtnOjEsIGY6Mn1d OworZm9yICh2YXIgaSA9IDA7IGkgPCAxMDAwMDsgKytpKSB7CisgICAgdmFyIHJlc3VsdCA9IGZv byhhcnJheVtpICUgYXJyYXkubGVuZ3RoXSk7CisgICAgaWYgKHJlc3VsdCAhPSA0MikKKyAgICAg ICAgdGhyb3cgIkVycm9yOiBiYWQgcmVzdWx0IGluIGxvb3A6ICIgKyByZXN1bHQ7Cit9CisKK3Zh ciBvID0ge307Cit2YXIgZGlkQ2FsbEdldHRlciA9IGZhbHNlOworby5fX2RlZmluZUdldHRlcl9f KCJmIiwgZnVuY3Rpb24oKSB7IGRpZENhbGxHZXR0ZXIgPSB0cnVlOyByZXR1cm4gNzM7IH0pOwor dmFyIHJlc3VsdCA9IGZvbyhvKTsKK2lmIChyZXN1bHQgIT0gNDIpCisgICAgdGhyb3cgIkVycm9y OiBiYWQgcmVzdWx0IGF0IGVuZDogIiArIHJlc3VsdDsKK2lmICghZGlkQ2FsbEdldHRlcikKKyAg ICB0aHJvdyAiRXJyb3I6IGRpZCBub3QgY2FsbCBnZXR0ZXIgYXQgZW5kLiI7Cg==