[Webkit-unassigned] [Bug 172431] New: The way the AI rule is written for CompareLess/etc is confusing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=172431

            Bug ID: 172431
           Summary: The way the AI rule is written for CompareLess/etc is
                    confusing
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: JavaScriptCore
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: sbarati at apple.com
                CC: benjamin at webkit.org, fpizlo at apple.com,
                    ggaren at apple.com, gskachkov at gmail.com,
                    jfbastien at apple.com, keith_miller at apple.com,
                    mark.lam at apple.com, msaboff at apple.com,
                    ticaiolima at gmail.com, utatane.tea at gmail.com

e.g:
```
    case CompareLess:
    case CompareLessEq:
    case CompareGreater:
    case CompareGreaterEq:
    case CompareEq: {
        JSValue leftConst = forNode(node->child1()).value();
        JSValue rightConst = forNode(node->child2()).value();
        if (leftConst && rightConst) {
            if (leftConst.isNumber() && rightConst.isNumber()) {
                double a = leftConst.asNumber();
                double b = rightConst.asNumber();
                switch (node->op()) {
                case CompareLess:
                    setConstant(node, jsBoolean(a < b));
                    break;
                case CompareLessEq:
                    setConstant(node, jsBoolean(a <= b));
                    break;
                case CompareGreater:
                    setConstant(node, jsBoolean(a > b));
                    break;
                case CompareGreaterEq:
                    setConstant(node, jsBoolean(a >= b));
                    break;
                case CompareEq:
                    setConstant(node, jsBoolean(a == b));
                    break;
                default:
                    RELEASE_ASSERT_NOT_REACHED();
                    break;
                }
                break;
            }

            if (leftConst.isString() && rightConst.isString()) {
                const StringImpl* a = asString(leftConst)->tryGetValueImpl();
                const StringImpl* b = asString(rightConst)->tryGetValueImpl();
                if (a && b) {
                    bool result;
                    if (node->op() == CompareEq)
                        result = WTF::equal(a, b);
                    else if (node->op() == CompareLess)
                        result = codePointCompare(a, b) < 0;
                    else if (node->op() == CompareLessEq)
                        result = codePointCompare(a, b) <= 0;
                    else if (node->op() == CompareGreater)
                        result = codePointCompare(a, b) > 0;
                    else if (node->op() == CompareGreaterEq)
                        result = codePointCompare(a, b) >= 0;
                    else
                        RELEASE_ASSERT_NOT_REACHED();
                    setConstant(node, jsBoolean(result));
                    break;
                }
            }

            if (node->op() == CompareEq && leftConst.isSymbol() && rightConst.isSymbol()) {
                setConstant(node, jsBoolean(asSymbol(leftConst) == asSymbol(rightConst)));
                break;
            }
        }

        if (node->op() == CompareEq) {
            SpeculatedType leftType = forNode(node->child1()).m_type;
            SpeculatedType rightType = forNode(node->child2()).m_type;
            if (!valuesCouldBeEqual(leftType, rightType)) {
                setConstant(node, jsBoolean(false));
                break;
            }

            if (leftType == SpecOther)
                std::swap(leftType, rightType);
            if (rightType == SpecOther) {
                // Undefined and Null are always equal when compared to eachother.
                if (!(leftType & ~SpecOther)) {
                    setConstant(node, jsBoolean(true));
                    break;
                }

                // Any other type compared to Null or Undefined is always false
                // as long as the MasqueradesAsUndefined watchpoint is valid.
                //
                // MasqueradesAsUndefined only matters for SpecObjectOther, other
                // cases are always "false".
                if (!(leftType & (SpecObjectOther | SpecOther))) {
                    setConstant(node, jsBoolean(false));
                    break;
                }

                if (!(leftType & SpecOther) && m_graph.masqueradesAsUndefinedWatchpointIsStillValid(node->origin.semantic)) {
                    JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
                    m_graph.watchpoints().addLazily(globalObject->masqueradesAsUndefinedWatchpoint());
                    setConstant(node, jsBoolean(false));
                    break;
                }
            }
        }

        if (node->child1() == node->child2()) {
            if (node->isBinaryUseKind(Int32Use) ||
                node->isBinaryUseKind(Int52RepUse) ||
                node->isBinaryUseKind(StringUse) ||
                node->isBinaryUseKind(BooleanUse) ||
                node->isBinaryUseKind(SymbolUse) ||
                node->isBinaryUseKind(StringIdentUse) ||
                node->isBinaryUseKind(ObjectUse) ||
                node->isBinaryUseKind(ObjectUse, ObjectOrOtherUse) ||
                node->isBinaryUseKind(ObjectOrOtherUse, ObjectUse)) {
                switch (node->op()) {
                case CompareLess:
                case CompareGreater:
                    setConstant(node, jsBoolean(false));
                    break;
                case CompareLessEq:
                case CompareGreaterEq:
                case CompareEq:
                    setConstant(node, jsBoolean(true));
                    break;
                default:
                    DFG_CRASH(m_graph, node, "Unexpected node type");
                    break;
                }
                break;
            }
        }

        forNode(node).setType(SpecBoolean);
        break;
    }
```

the node->child1() == node->child2() branch looks clearly wrong, e.g, if you have CompareLess with BinaryUseKind(Object) since we would be skipping over valueOf().
I don't think we ever actually allow such binary use kind for non CompareEq given the rule in FixupPhase, but anyways, we should probably be more specific about effects in JS here.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170521/5a6b2238/attachment-0001.html>