REGRESSION (r227341): DFG_ASSERT failure at JSC::DFG::AtTailAbstractState::forNode()

[Michael Saboff]

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.

[Michael Saboff]

<rdar://problem/36833641>

[Michael Saboff]

Created attachment 332553 [details]
Patch

[Keith Miller]

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...

[Michael Saboff]

Created attachment 332556 [details]
Patch with updates from review

[Keith Miller]

Comment on attachment 332556 [details]
Patch with updates from review

r=me.

[WebKit Commit Bot]

Comment on attachment 332556 [details]
Patch with updates from review

Clearing flags on attachment: 332556

Committed r227742: <https://trac.webkit.org/changeset/227742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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:).