Bug 172431 - The way the AI rule is written for CompareLess/etc is confusing
Summary: The way the AI rule is written for CompareLess/etc is confusing
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-21 15:49 PDT by Saam Barati
Modified: 2017-05-21 15:49 PDT (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-05-21 15:49:16 PDT
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.