Bug 156378 – tryGetById should be supported by the DFG/FTL

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

tryGetById should be supported by the DFG/FTL

[Keith Miller]

Created attachment 275959 [details]
Patch

[Geoffrey Garen]

/Volumes/Data/EWS/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:979:66: error: cannot initialize a parameter of type 'GPRReg' (aka 'RegisterID') with an rvalue of type 'unsigned int'
        cachedGetById(node->origin.semantic, baseGPR, resultGPR, node->identifierNumber(), JITCompiler::Jump(), DontSpill, AccessType::GetPure);
                                                                 ^~~~~~~~~~~~~~~~~~~~~~~~

[Keith Miller]

Created attachment 276043 [details]
Patch

[Mark Lam]

Comment on attachment 276043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276043&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        we do not profile the result type. This profiling is unnessary for the current

typo: /unnessary/unnecessary/.

[Filip Pizlo]

Comment on attachment 276043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276043&action=review

I think you should reduce the code duplication in ByteCodeParser.

> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:78
> -    if (instruction[0].u.opcode == LLInt::getOpcode(op_get_array_length))
> +    if (instruction[0].u.opcode == LLInt::getOpcode(op_get_array_length) || instruction[0].u.opcode == LLInt::getOpcode(op_try_get_by_id))

Can you provide clarity on why you return NoInformation when LLInt saw a try_get_by_id?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1969
> +        forNode(node).makeBytecodeTop();

Why isn't this makeHeapTop()?  Can TryGetById return the empty value?  If not, then this should be HeapTop.

Also, it seems like you should add folding here that is like the folding we do in GetById.  If you don't add it, then please add a FIXME to that effect and link it to a bug!

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2889
> +void ByteCodeParser::handleTryGetById(int destinationOperand, Node* base, unsigned identifierNumber, const GetByIdStatus& getByIdStatus)

This method looks like it duplicates a *ton* of code with handleGetById.  Please try to make them common.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2896
> +    if (!getByIdStatus.isSimple() || !getByIdStatus.numVariants() || !Options::useAccessInlining()) {
> +        set(VirtualRegister(destinationOperand),
> +            addToGraph(TryGetById, OpInfo(identifierNumber), OpInfo(SpecBytecodeTop), base));
> +        return;
> +    }

This is identical to the first case in handleGetById.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2904
> +        if (!isFTL(m_graph.m_plan.mode)
> +            || !Options::usePolymorphicAccessInlining()) {
> +            set(VirtualRegister(destinationOperand),
> +                addToGraph(TryGetById, OpInfo(identifierNumber), OpInfo(SpecBytecodeTop), base));
> +            return;
> +        }

Identical.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2932
> +        // 1) Emit prototype structure checks for all chains. This could sort of maybe not be
> +        //    optimal, if there is some rarely executed case in the chain that requires a lot
> +        //    of checks and those checks are not watchpointable.
> +        for (const GetByIdVariant& variant : getByIdStatus.variants()) {
> +            ASSERT(variant.intrinsic() == NoIntrinsic);
> +
> +            if (variant.conditionSet().isEmpty()) {
> +                cases.append(MultiGetByOffsetCase(
> +                    variant.structureSet(),
> +                    GetByOffsetMethod::load(variant.offset())));
> +                continue;
> +            }
> +
> +            GetByOffsetMethod method = planLoad(variant.conditionSet());
> +            if (!method) {
> +                set(VirtualRegister(destinationOperand),
> +                    addToGraph(TryGetById, OpInfo(identifierNumber), OpInfo(SpecBytecodeTop), base));
> +                return;
> +            }
> +
> +            cases.append(MultiGetByOffsetCase(variant.structureSet(), method));
> +        }
> +
> +        if (m_graph.compilation())
> +            m_graph.compilation()->noticeInlinedGetById();

So very identical.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2940
> +        // 2) Emit a MultiGetByOffset
> +        MultiGetByOffsetData* data = m_graph.m_multiGetByOffsetData.add();
> +        data->cases = cases;
> +        data->identifierNumber = identifierNumber;
> +        set(VirtualRegister(destinationOperand),
> +            addToGraph(MultiGetByOffset, OpInfo(data), OpInfo(SpecBytecodeTop), base));
> +        return;

Identical.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2957
> +    if (m_graph.compilation())
> +        m_graph.compilation()->noticeInlinedGetById();
> +    set(VirtualRegister(destinationOperand), loadedValue);
> +}

Looks like this is the only place where this is different from handleGetById.  Basically, you want handleGetById to have a flag or something that tells it that it's the Try variant, and then you want handleGetById to return early here.

> Source/JavaScriptCore/dfg/DFGClobberize.h:859
> +    case TryGetById: {
> +        read(JSCell_structureID);
> +        read(JSObject_butterfly);
> +        AbstractHeap heap(NamedProperties, node->identifierNumber());
> +        read(heap);
> +        def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(node));
> +        return;
> +    }

Are you sure about this?  Can't TryGetById call any arbitrary GetOwnPropertySlot?

I think that the more conservative thing to do for now is remove the def() and make this read(Heap).

[Keith Miller]

Comment on attachment 276043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276043&action=review

>> Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:78
>> +    if (instruction[0].u.opcode == LLInt::getOpcode(op_get_array_length) || instruction[0].u.opcode == LLInt::getOpcode(op_try_get_by_id))
> 
> Can you provide clarity on why you return NoInformation when LLInt saw a try_get_by_id?

Because we don't have an LLInt cache for TryGetById. In the long run I do think that an LLInt cache should be added.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1969
>> +        forNode(node).makeBytecodeTop();
> 
> Why isn't this makeHeapTop()?  Can TryGetById return the empty value?  If not, then this should be HeapTop.
> 
> Also, it seems like you should add folding here that is like the folding we do in GetById.  If you don't add it, then please add a FIXME to that effect and link it to a bug!

Yeah, it should be HeapTop. I will fix.

The folding that we do here will not help any current uses of this operation since they all use values on the prototype. I'll create a bug though.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2889
>> +void ByteCodeParser::handleTryGetById(int destinationOperand, Node* base, unsigned identifierNumber, const GetByIdStatus& getByIdStatus)
> 
> This method looks like it duplicates a *ton* of code with handleGetById.  Please try to make them common.

Yeah, you are probably right. I will change it.

>> Source/JavaScriptCore/dfg/DFGClobberize.h:859
>> +    }
> 
> Are you sure about this?  Can't TryGetById call any arbitrary GetOwnPropertySlot?
> 
> I think that the more conservative thing to do for now is remove the def() and make this read(Heap).

Changed.

[Keith Miller]

Created attachment 276051 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 276051 [details]
Patch

Clearing flags on attachment: 276051

Committed r199279: <http://trac.webkit.org/changeset/199279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 276051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276051&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3867
> +            handleGetById(currentInstruction[1].u.operand, SpecHeapTop, base, identifierNumber, getByIdStatus, AccessType::GetPure);

You should be able to produce a type prediction here.

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

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:177
> +            changed |= setPrediction(SpecBytecodeTop);

It's wrong to ever use any of the Top's in prediction propagation phase.  This tells the prediction propagator that this node will *definitely* return values of all types, which causes us to assume the worst for all downstream code.

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