For things like: https://bugs.webkit.org/show_bug.cgi?id=206805 For example, GetButterfly should probably be checking that its base is an object
<rdar://problem/59085094>
(In reply to Saam Barati from comment #0) > For things like: > https://bugs.webkit.org/show_bug.cgi?id=206805 > > For example, GetButterfly should probably be checking that its base is an > object Isn’t that covered by safeToExecute’s edge processing?
(In reply to Filip Pizlo from comment #2) > (In reply to Saam Barati from comment #0) > > For things like: > > https://bugs.webkit.org/show_bug.cgi?id=206805 > > > > For example, GetButterfly should probably be checking that its base is an > > object > > Isn’t that covered by safeToExecute’s edge processing? GetButterfly only speculates Cell on its base.
(In reply to Saam Barati from comment #3) > (In reply to Filip Pizlo from comment #2) > > (In reply to Saam Barati from comment #0) > > > For things like: > > > https://bugs.webkit.org/show_bug.cgi?id=206805 > > > > > > For example, GetButterfly should probably be checking that its base is an > > > object > > > > Isn’t that covered by safeToExecute’s edge processing? > > GetButterfly only speculates Cell on its base. Probably it should speculate Object.
Created attachment 400984 [details] patch
Comment on attachment 400984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400984&action=review > Source/JavaScriptCore/tools/JSDollarVM.cpp:1220 > +static const DOMJIT::Signature DOMJITFunctionObjectSignature(DOMJITFunctionObject::functionWithoutTypeCheck, DOMJITFunctionObject::info(), DOMJIT::Effect::forRead(DOMJIT::HeapRange(DOMJIT::HeapRange::ConstExpr, 0, 1)), SpecInt32Only); we only had one test using this functionality, and it didn't rely on the old behavior, so I just changed it to this so I didn't have to create a new class.
Comment on attachment 400984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=400984&action=review r=me > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487 > + bool isSafe = true; > + const ClassInfo* classInfo = node->requiredDOMJITClassInfo(); > + structures.forEach([&] (RegisteredStructure structure) { > + isSafe &= structure->classInfo()->isSubClassOf(classInfo); > + }); > + return isSafe; > + } What happens if structure set is empty?
(In reply to Yusuke Suzuki from comment #7) > Comment on attachment 400984 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400984&action=review > > r=me > > > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487 > > + bool isSafe = true; > > + const ClassInfo* classInfo = node->requiredDOMJITClassInfo(); > > + structures.forEach([&] (RegisteredStructure structure) { > > + isSafe &= structure->classInfo()->isSubClassOf(classInfo); > > + }); > > + return isSafe; > > + } > > What happens if structure set is empty? We should never reach such a point in the program, so we can return true or false.
(In reply to Saam Barati from comment #8) > (In reply to Yusuke Suzuki from comment #7) > > Comment on attachment 400984 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=400984&action=review > > > > r=me > > > > > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487 > > > + bool isSafe = true; > > > + const ClassInfo* classInfo = node->requiredDOMJITClassInfo(); > > > + structures.forEach([&] (RegisteredStructure structure) { > > > + isSafe &= structure->classInfo()->isSubClassOf(classInfo); > > > + }); > > > + return isSafe; > > > + } > > > > What happens if structure set is empty? > > We should never reach such a point in the program, so we can return true or > false. I think empty set means you have to return false.
(In reply to Saam Barati from comment #4) > (In reply to Saam Barati from comment #3) > > (In reply to Filip Pizlo from comment #2) > > > (In reply to Saam Barati from comment #0) > > > > For things like: > > > > https://bugs.webkit.org/show_bug.cgi?id=206805 > > > > > > > > For example, GetButterfly should probably be checking that its base is an > > > > object > > > > > > Isn’t that covered by safeToExecute’s edge processing? > > > > GetButterfly only speculates Cell on its base. > > Probably it should speculate Object. Oh interesting. If we said it speculates object then we would really want it to be like KnownObject - so safe to execute if known object, but never actually checks it.
(In reply to Filip Pizlo from comment #10) > (In reply to Saam Barati from comment #4) > > (In reply to Saam Barati from comment #3) > > > (In reply to Filip Pizlo from comment #2) > > > > (In reply to Saam Barati from comment #0) > > > > > For things like: > > > > > https://bugs.webkit.org/show_bug.cgi?id=206805 > > > > > > > > > > For example, GetButterfly should probably be checking that its base is an > > > > > object > > > > > > > > Isn’t that covered by safeToExecute’s edge processing? > > > > > > GetButterfly only speculates Cell on its base. > > > > Probably it should speculate Object. > > Oh interesting. If we said it speculates object then we would really want it > to be like KnownObject - so safe to execute if known object, but never > actually checks it. Yeah exactly. I fixed this issue a while ago where we only hoist GetButterfly if we prove the base is an object. KnownObject probably sounds like the better long-term thing since it's what we really mean anyways
(In reply to Filip Pizlo from comment #9) > (In reply to Saam Barati from comment #8) > > (In reply to Yusuke Suzuki from comment #7) > > > Comment on attachment 400984 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=400984&action=review > > > > > > r=me > > > > > > > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:487 > > > > + bool isSafe = true; > > > > + const ClassInfo* classInfo = node->requiredDOMJITClassInfo(); > > > > + structures.forEach([&] (RegisteredStructure structure) { > > > > + isSafe &= structure->classInfo()->isSubClassOf(classInfo); > > > > + }); > > > > + return isSafe; > > > > + } > > > > > > What happens if structure set is empty? > > > > We should never reach such a point in the program, so we can return true or > > false. > > I think empty set means you have to return false. Doesn't the empty set prove we would have exitted?
Committed r262642: <https://trac.webkit.org/changeset/262642> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400984 [details].