106074 2013-01-03 18:52:35 -0800 DFG should not elide CheckStructure if it's needed to perform a cell check 2013-01-04 15:42:19 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 fpizlo fpizlo barraclough ggaren mark.lam mhahnenberg msaboff oliver sam oldest_to_newest 799894 0 fpizlo 2013-01-03 18:52:35 -0800 CheckStructure checks two things: if the value is a cell, and if so, that it has a particular structure (or one of a set of particular structures). We shouldn't elide the CheckStructure just because we know that if the value was a cell then it would have a particular structure; we need to also prove that it is a cell. (Example: we might have proved that a value is either a boolean or a cell with the primordial int32 array structure.) Of course, it would be even better in those cases to replace CheckStructure with CheckCell. But for now it's best to just fix the bug. 799895 1 181262 fpizlo 2013-01-03 18:53:18 -0800 Created attachment 181262 the patch 799896 2 181262 rniwa 2013-01-03 19:03:45 -0800 Comment on attachment 181262 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here. > Source/JavaScriptCore/ChangeLog:3 > + Please add the bug title here. > Source/JavaScriptCore/ChangeLog:9 > + * dfg/DFGConstantFoldingPhase.cpp: > + (JSC::DFG::ConstantFoldingPhase::foldConstants): It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence). > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107 > + if (!isCellSpeculation(value.m_type)) > + break; > if (value.m_currentKnownStructure.isSubsetOf(set)) { I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true. 799897 3 fpizlo 2013-01-03 19:04:52 -0800 (In reply to comment #2) > (From update of attachment 181262 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review > > r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here. > > > Source/JavaScriptCore/ChangeLog:3 > > + > > Please add the bug title here. > > > Source/JavaScriptCore/ChangeLog:9 > > + * dfg/DFGConstantFoldingPhase.cpp: > > + (JSC::DFG::ConstantFoldingPhase::foldConstants): > > It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence). > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107 > > + if (!isCellSpeculation(value.m_type)) > > + break; > > if (value.m_currentKnownStructure.isSubsetOf(set)) { > > I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true. Or even better, just returns false is isCellSpeculation(m_type) is false. So we don't have to repeat the idiom of checking both things in the future. 799898 4 fpizlo 2013-01-03 19:06:34 -0800 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 181262 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=181262&action=review > > > > r=me. Despite the fact I’m not an expert of JSC, I think Filip explained me enough details to know what’s happening here. > > > > > Source/JavaScriptCore/ChangeLog:3 > > > + > > > > Please add the bug title here. > > > > > Source/JavaScriptCore/ChangeLog:9 > > > + * dfg/DFGConstantFoldingPhase.cpp: > > > + (JSC::DFG::ConstantFoldingPhase::foldConstants): > > > > It’ll be great if you could explain what you’ve described me in person (probably in some brief sentence). > > > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:107 > > > + if (!isCellSpeculation(value.m_type)) > > > + break; > > > if (value.m_currentKnownStructure.isSubsetOf(set)) { > > > > I wonder if you can add isSubsetOf that takes value and ASSERT that isCellSpeculation or whatever condition needed is true. > > Or even better, just returns false is isCellSpeculation(m_type) is false. So we don't have to repeat the idiom of checking both things in the future. I think I'll do that refactoring separately. I'm now starting to think about how to more broadly rationalize the AbstractValue API, so I'll hold off until I get a better idea in my head. 800698 5 fpizlo 2013-01-04 15:42:19 -0800 Landed in http://trac.webkit.org/changeset/138862 181262 2013-01-03 18:53:18 -0800 2013-01-03 19:03:45 -0800 the patch fixcell.patch text/plain 1842 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTM4NzcxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDEzLTAxLTAzICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDYwNzQK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGRmZy9E RkdDb25zdGFudEZvbGRpbmdQaGFzZS5jcHA6CisgICAgICAgIChKU0M6OkRGRzo6Q29uc3RhbnRG b2xkaW5nUGhhc2U6OmZvbGRDb25zdGFudHMpOgorCiAyMDEzLTAxLTAzICBGaWxpcCBQaXpsbyAg PGZwaXpsb0BhcHBsZS5jb20+CiAKICAgICAgICAgREZHOjpCeXRlQ29kZUNhY2hlIHNlcnZlcyBs aXR0bGUgb3Igbm8gcHVycG9zZSBldmVyIHNpbmNlIHdlIGRlY2lkZWQgdG8ga2VlcCBieXRlY29k ZSBhcm91bmQgcGVybWFuZW50bHkKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZH Q29uc3RhbnRGb2xkaW5nUGhhc2UuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9kZmcvREZHQ29uc3RhbnRGb2xkaW5nUGhhc2UuY3BwCShyZXZpc2lvbiAxMzg3NjMpCisr KyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0NvbnN0YW50Rm9sZGluZ1BoYXNlLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMTAyLDYgKzEwMiw4IEBAIHByaXZhdGU6CiAgICAgICAgICAgICAg ICAgICAgIHNldCA9IG5vZGUuc3RydWN0dXJlKCk7CiAgICAgICAgICAgICAgICAgZWxzZQogICAg ICAgICAgICAgICAgICAgICBzZXQgPSBub2RlLnN0cnVjdHVyZVNldCgpOworICAgICAgICAgICAg ICAgIGlmICghaXNDZWxsU3BlY3VsYXRpb24odmFsdWUubV90eXBlKSkKKyAgICAgICAgICAgICAg ICAgICAgYnJlYWs7CiAgICAgICAgICAgICAgICAgaWYgKHZhbHVlLm1fY3VycmVudEtub3duU3Ry dWN0dXJlLmlzU3Vic2V0T2Yoc2V0KSkgewogICAgICAgICAgICAgICAgICAgICBBU1NFUlQobm9k ZS5yZWZDb3VudCgpID09IDEpOwogICAgICAgICAgICAgICAgICAgICBub2RlLnNldE9wQW5kRGVm YXVsdEZsYWdzKFBoYW50b20pOwpAQCAtMTEwLDggKzExMiw3IEBAIHByaXZhdGU6CiAgICAgICAg ICAgICAgICAgfQogICAgICAgICAgICAgICAgIFN0cnVjdHVyZUFic3RyYWN0VmFsdWUmIHN0cnVj dHVyZVZhbHVlID0gdmFsdWUubV9mdXR1cmVQb3NzaWJsZVN0cnVjdHVyZTsKICAgICAgICAgICAg ICAgICBpZiAoc3RydWN0dXJlVmFsdWUuaXNTdWJzZXRPZihzZXQpCi0gICAgICAgICAgICAgICAg ICAgICYmIHN0cnVjdHVyZVZhbHVlLmhhc1NpbmdsZXRvbigpCi0gICAgICAgICAgICAgICAgICAg ICYmIGlzQ2VsbFNwZWN1bGF0aW9uKHZhbHVlLm1fdHlwZSkpIHsKKyAgICAgICAgICAgICAgICAg ICAgJiYgc3RydWN0dXJlVmFsdWUuaGFzU2luZ2xldG9uKCkpIHsKICAgICAgICAgICAgICAgICAg ICAgbm9kZS5jb252ZXJ0VG9TdHJ1Y3R1cmVUcmFuc2l0aW9uV2F0Y2hwb2ludChzdHJ1Y3R1cmVW YWx1ZS5zaW5nbGV0b24oKSk7CiAgICAgICAgICAgICAgICAgICAgIGNoYW5nZWQgPSB0cnVlOwog ICAgICAgICAgICAgICAgIH0K