Change set r227341 (<https://trac.webkit.org/changeset/227341>) changed the abstract interpreter for the non-strict Compare nodes like CompareEq. That change caused a DFG_ASSERT() to fire: DFG ASSERTION FAILED: iter != valuesAtTail.end() ./dfg/DFGAtTailAbstractState.cpp(59) : JSC::DFG::AbstractValue &JSC::DFG::AtTailAbstractState::forNode(JSC::DFG::NodeFlowProjection) There needs to be a corresponding change in clobberize() for UntypedUse as well.
<rdar://problem/36833641>
Created attachment 332553 [details] Patch
Comment on attachment 332553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332553&action=review > Source/JavaScriptCore/dfg/DFGClobberize.h:1550 > + if (node->child1().useKind() == UntypedUse || node->child1().useKind() == ObjectUse > + || node->child2().useKind() == UntypedUse || node->child2().useKind() == ObjectUse) { You should have a check for node->isBinaryUseKind(ObjectUse) since that won't convert. Nit: Also, I would make a local for the useKinds...
Created attachment 332556 [details] Patch with updates from review
Comment on attachment 332556 [details] Patch with updates from review r=me.
Comment on attachment 332556 [details] Patch with updates from review Clearing flags on attachment: 332556 Committed r227742: <https://trac.webkit.org/changeset/227742>
All reviewed patches have been landed. Closing bug.
Comment on attachment 332556 [details] Patch with updates from review View in context: https://bugs.webkit.org/attachment.cgi?id=332556&action=review > Source/JavaScriptCore/dfg/DFGClobberize.h:1560 > - if (!node->isBinaryUseKind(UntypedUse)) { > + > + if (node->op() == CompareEq && node->isBinaryUseKind(ObjectUse)) { > def(PureValue(node)); > return; > } > - read(World); > - write(Heap); > + if (node->child1().useKind() == UntypedUse || node->child1().useKind() == ObjectUse > + || node->child2().useKind() == UntypedUse || node->child2().useKind() == ObjectUse) { > + read(World); > + write(Heap); > + return; > + } > + > + def(PureValue(node)); This patch creates a major discrepancy between AI's handling of CompareEq and clobberize's handling of CompareEq. Also, from what I can tell, it's only CompareEq(Untyped:, Untyped:) that can have effects. CompareEq(ObjectUse:, _) cannot have effects.
I don't think it's correct to fix assertions in AtTailAbstractState by pretending that something is effectful. It seems that all that this patch did was mask a more fundamental bug by turning off hoisting of CompareEq in this case. But CompareEq is effectless here, so if hoisting it causes a problem then pretending that it's effectful is not the right solution.
I see the problem now: in r227341, you incorrectly made ComapreEq(Untyped:, Other:) pretend to have effect even though it doesn't. But this means that attempting to hoist it causes clobberWorld() to get called with AtTailAbstractState, which causes crashes. The correct solution is to fix AI so that it's precise about the effectfulness of CompareEq(Untyped:, Other:).