Bug 210882 – Fix OSR exiting/iterator object checks in for-of bytecodes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Fix OSR exiting/iterator object checks in for-of bytecodes

[Keith Miller]

Created attachment 397279 [details]
Patch

[Keith Miller]

rdar://problem/62094397

[Mark Lam]

Comment on attachment 397279 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:180
> +        Operand result, NodeType op, InlineCallFrame::Kind, BytecodeIndex nextIndex,

Let's call this osrExitIndex instead of nextIndex.  Ditto for argument names below for each of the handler functions.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:184
> +    Terminality handleCall(const Instruction* pc, NodeType op, CallMode, BytecodeIndex nextIndex = BytecodeIndex());

nit: Is it possible to use a default arg of osrExitIndex = nextBytecodeIndex() here?  If not, can we just not have a default argument and pass nextBytecodeIndex() at the 5 locations that calls this function?  A default osrExitIndex of "nowhere" as implied by BytecodeIndex() just doesn't read right to me.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:295
> +    BytecodeIndex nextBytecodeIndex() const { return BytecodeIndex(m_currentIndex.offset() + m_currentInstruction->size()); }

I'm not in love with this name, mostly because "Bytecode" is so overloaded in our usage.  nextBytecodeIndex can mean the (1) next bytecode instruction boundary's index, or (2) the next thing that can be a BytecodeIndex.  How about naming this nextBytecodeInstructionIndex() or nextBytecodeBoundaryIndex() or nextBytecodeOpcodeIndex() instead?  Personally, I prefer nextBytecodeOpcodeIndex() because it's the shortest and reads as (1) the index of the next bytecode's opcode, or (2) the index of the next bytecode instruction, both of which conveys exactly what this function returns.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1323
> +    if (!nextIndex)
> +        nextIndex = nextBytecodeIndex();

Please remove this if you can address the default argument suggestion I noted in my nit above.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6898
> +                    Node* doneIndex = jsConstant(jsNumber(-1));
> +                    Node* index = addToGraph(GetInternalField, OpInfo(static_cast<uint32_t>(JSArrayIterator::Field::Index)), OpInfo(SpecInt32Only), iterator);
> +                    Node* isDone = addToGraph(CompareStrictEq, index, doneIndex);
>  
> -                    Node* index = addToGraph(GetInternalField, OpInfo(static_cast<uint32_t>(JSArrayIterator::Field::Index)), OpInfo(SpecInt32Only), get(bytecode.m_iterator));
>                      Node* iterable = get(bytecode.m_iterable);
>                      Node* butterfly = addToGraph(GetButterfly, iterable);
>                      Node* length = addToGraph(GetArrayLength, OpInfo(arrayMode.asWord()), iterable, butterfly);
> -                    Node* isDone = addToGraph(CompareGreaterEq, Edge(index, Int32Use), Edge(length, Int32Use));
> -                    m_exitOK = true; // The above compare doesn't produce effects since we know index and length are int32s.
> +                    Node* isOutOfBounds = addToGraph(CompareGreaterEq, Edge(index, Int32Use), Edge(length, Int32Use));
> +
> +                    isDone = addToGraph(ArithBitOr, isDone, isOutOfBounds);
> +                    // The above compare doesn't produce effects since we know the values are booleans. We don't set UseKinds because Fixup likes to add edges.

You're still doing 2 compares and then Or'ing the boolean results.  Why not just do an unsigned compare of index against length?  If we're done, index == -1 which is UINT_MAX, and would be greater than length because length can only be INT_MAX at most, right?

[Keith Miller]

Comment on attachment 397279 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:180
>> +        Operand result, NodeType op, InlineCallFrame::Kind, BytecodeIndex nextIndex,
> 
> Let's call this osrExitIndex instead of nextIndex.  Ditto for argument names below for each of the handler functions.

Sure, changed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:184
>> +    Terminality handleCall(const Instruction* pc, NodeType op, CallMode, BytecodeIndex nextIndex = BytecodeIndex());
> 
> nit: Is it possible to use a default arg of osrExitIndex = nextBytecodeIndex() here?  If not, can we just not have a default argument and pass nextBytecodeIndex() at the 5 locations that calls this function?  A default osrExitIndex of "nowhere" as implied by BytecodeIndex() just doesn't read right to me.

No, you can't call a member function in a default parameter... idk why, sadly. But I can change it to not have a default parameter, I both assumed there were more callsites and wanted to maintain consistency with handleVarargsCall below.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:295
>> +    BytecodeIndex nextBytecodeIndex() const { return BytecodeIndex(m_currentIndex.offset() + m_currentInstruction->size()); }
> 
> I'm not in love with this name, mostly because "Bytecode" is so overloaded in our usage.  nextBytecodeIndex can mean the (1) next bytecode instruction boundary's index, or (2) the next thing that can be a BytecodeIndex.  How about naming this nextBytecodeInstructionIndex() or nextBytecodeBoundaryIndex() or nextBytecodeOpcodeIndex() instead?  Personally, I prefer nextBytecodeOpcodeIndex() because it's the shortest and reads as (1) the index of the next bytecode's opcode, or (2) the index of the next bytecode instruction, both of which conveys exactly what this function returns.

How about nextOpcodeIndex() If I had to pick something wordier I would say nextOpcodesBytecodeIndex() but I'm not sure that's more meaningful since we use index fairly regularly to mean the bytecodeIndex.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1323
>> +        nextIndex = nextBytecodeIndex();
> 
> Please remove this if you can address the default argument suggestion I noted in my nit above.

Done.

[Keith Miller]

Created attachment 397311 [details]
Patch

[Keith Miller]

Created attachment 397321 [details]
Patch

[Keith Miller]

Created attachment 397358 [details]
Patch

[Mark Lam]

Comment on attachment 397358 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:20
> +        iterator's closed state (index == -1) and index is out of bounds. We can't
> +        do a CompareBelow check because the index is effectively an int33_t.

Why not?  CompareBelow is for comparing int32_t (btw, typo with int33_t here).

    case CompareBelow:
        compileCompareUnsigned(node, JITCompiler::Below);

void SpeculativeJIT::compileInt32Compare(Node* node, MacroAssembler::RelationalCondition condition)
{
    if (node->child1()->isInt32Constant()) {
        SpeculateInt32Operand op2(this, node->child2());
        GPRTemporary result(this, Reuse, op2);
        int32_t imm = node->child1()->asInt32();
        m_jit.compare32(condition, JITCompiler::Imm32(imm), op2.gpr(), result.gpr());

        unblessedBooleanResult(result.gpr(), node);
    } else if (node->child2()->isInt32Constant()) {
        SpeculateInt32Operand op1(this, node->child1());
        GPRTemporary result(this, Reuse, op1);
        int32_t imm = node->child2()->asInt32();
        m_jit.compare32(condition, op1.gpr(), JITCompiler::Imm32(imm), result.gpr());

        unblessedBooleanResult(result.gpr(), node);
    } else {
        SpeculateInt32Operand op1(this, node->child1());  // <=============== int32_t
        SpeculateInt32Operand op2(this, node->child2()); // <=============== int32_t
        GPRTemporary result(this, Reuse, op1, op2);
        m_jit.compare32(condition, op1.gpr(), op2.gpr(), result.gpr());

        unblessedBooleanResult(result.gpr(), node);
    }
}

What am I missing?

[Mark Lam]

(In reply to Mark Lam from comment #8)
> Comment on attachment 397358 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397358&action=review
> 
> > Source/JavaScriptCore/ChangeLog:20
> > +        iterator's closed state (index == -1) and index is out of bounds. We can't
> > +        do a CompareBelow check because the index is effectively an int33_t.
> 
> Why not?  CompareBelow is for comparing int32_t (btw, typo with int33_t
> here).
> 
>     case CompareBelow:
>         compileCompareUnsigned(node, JITCompiler::Below);
> 
> void SpeculativeJIT::compileInt32Compare(Node* node,
> MacroAssembler::RelationalCondition condition)
> {
>     if (node->child1()->isInt32Constant()) {
>         SpeculateInt32Operand op2(this, node->child2());
>         GPRTemporary result(this, Reuse, op2);
>         int32_t imm = node->child1()->asInt32();
>         m_jit.compare32(condition, JITCompiler::Imm32(imm), op2.gpr(),
> result.gpr());
> 
>         unblessedBooleanResult(result.gpr(), node);
>     } else if (node->child2()->isInt32Constant()) {
>         SpeculateInt32Operand op1(this, node->child1());
>         GPRTemporary result(this, Reuse, op1);
>         int32_t imm = node->child2()->asInt32();
>         m_jit.compare32(condition, op1.gpr(), JITCompiler::Imm32(imm),
> result.gpr());
> 
>         unblessedBooleanResult(result.gpr(), node);
>     } else {
>         SpeculateInt32Operand op1(this, node->child1());  //
> <=============== int32_t
>         SpeculateInt32Operand op2(this, node->child2()); // <===============
> int32_t
>         GPRTemporary result(this, Reuse, op1, op2);
>         m_jit.compare32(condition, op1.gpr(), op2.gpr(), result.gpr());
> 
>         unblessedBooleanResult(result.gpr(), node);
>     }
> }
> 
> What am I missing?

Forgot:

void SpeculativeJIT::compileCompareUnsigned(Node* node, MacroAssembler::RelationalCondition condition)
{
    compileInt32Compare(node, condition);
}

[Saam Barati]

Comment on attachment 397358 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:19
> +        Finally, this patch makes a small optimization where we just ArithBitOr the
> +        iterator's closed state (index == -1) and index is out of bounds. We can't

Is this actually faster?

> Source/JavaScriptCore/ChangeLog:20
> +        do a CompareBelow check because the index is effectively an int33_t.

int33 => int32_t

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:255
> -        bool isDirect, unsigned intructionSize, ECMAMode);
> +        bool isDirect, BytecodeIndex osrExitIndex, ECMAMode);

nice

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6887
> +                    Node* doneIndex = jsConstant(jsNumber(-1));

same comment as your original patch: can we make this a constant somewhere and use it by name? We hardcode -1 in a lot of places and it's not the best abstraction

[Keith Miller]

Comment on attachment 397358 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:19
>> +        iterator's closed state (index == -1) and index is out of bounds. We can't
> 
> Is this actually faster?

I didn't test it but it seems unlikely that the currently impossible case of getting a closed iterator is common enough to justify having a basic block for it.

>>>> Source/JavaScriptCore/ChangeLog:20
>>>> +        do a CompareBelow check because the index is effectively an int33_t.
>>> 
>>> Why not?  CompareBelow is for comparing int32_t (btw, typo with int33_t here).
>>> 
>>>     case CompareBelow:
>>>         compileCompareUnsigned(node, JITCompiler::Below);
>>> 
>>> void SpeculativeJIT::compileInt32Compare(Node* node, MacroAssembler::RelationalCondition condition)
>>> {
>>>     if (node->child1()->isInt32Constant()) {
>>>         SpeculateInt32Operand op2(this, node->child2());
>>>         GPRTemporary result(this, Reuse, op2);
>>>         int32_t imm = node->child1()->asInt32();
>>>         m_jit.compare32(condition, JITCompiler::Imm32(imm), op2.gpr(), result.gpr());
>>> 
>>>         unblessedBooleanResult(result.gpr(), node);
>>>     } else if (node->child2()->isInt32Constant()) {
>>>         SpeculateInt32Operand op1(this, node->child1());
>>>         GPRTemporary result(this, Reuse, op1);
>>>         int32_t imm = node->child2()->asInt32();
>>>         m_jit.compare32(condition, op1.gpr(), JITCompiler::Imm32(imm), result.gpr());
>>> 
>>>         unblessedBooleanResult(result.gpr(), node);
>>>     } else {
>>>         SpeculateInt32Operand op1(this, node->child1());  // <=============== int32_t
>>>         SpeculateInt32Operand op2(this, node->child2()); // <=============== int32_t
>>>         GPRTemporary result(this, Reuse, op1, op2);
>>>         m_jit.compare32(condition, op1.gpr(), op2.gpr(), result.gpr());
>>> 
>>>         unblessedBooleanResult(result.gpr(), node);
>>>     }
>>> }
>>> 
>>> What am I missing?
>> 
>> Forgot:
>> 
>> void SpeculativeJIT::compileCompareUnsigned(Node* node, MacroAssembler::RelationalCondition condition)
>> {
>>     compileInt32Compare(node, condition);
>> }
> 
> int33 => int32_t

It's int33 because GetArrayLength secretly returns an uint32_t so you can't mix it with a int32_t. To be fair I think it should work but for some reason doesn't today... I filed a bug to investigate later: https://bugs.webkit.org/show_bug.cgi?id=210927

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6887
>> +                    Node* doneIndex = jsConstant(jsNumber(-1));
> 
> same comment as your original patch: can we make this a constant somewhere and use it by name? We hardcode -1 in a lot of places and it's not the best abstraction

Done.

[Keith Miller]

This landed in r260585