Bug 210882

Summary: Fix OSR exiting/iterator object checks in for-of bytecodes
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch saam: review+

Description Keith Miller 2020-04-22 15:33:20 PDT
Fix OSR exiting/iterator object checks in for-of bytecodes
Comment 1 Keith Miller 2020-04-22 15:39:22 PDT
Created attachment 397279 [details]
Patch
Comment 2 Keith Miller 2020-04-22 15:58:31 PDT
rdar://problem/62094397
Comment 3 Mark Lam 2020-04-22 18:40:33 PDT
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?
Comment 4 Keith Miller 2020-04-22 20:27:19 PDT
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.
Comment 5 Keith Miller 2020-04-22 20:36:31 PDT
Created attachment 397311 [details]
Patch
Comment 6 Keith Miller 2020-04-22 22:37:24 PDT
Created attachment 397321 [details]
Patch
Comment 7 Keith Miller 2020-04-23 10:26:19 PDT
Created attachment 397358 [details]
Patch
Comment 8 Mark Lam 2020-04-23 10:51:32 PDT
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?
Comment 9 Mark Lam 2020-04-23 10:52:14 PDT
(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);
}
Comment 10 Saam Barati 2020-04-23 11:08:16 PDT
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
Comment 11 Keith Miller 2020-04-23 11:39:37 PDT
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.
Comment 12 Keith Miller 2020-04-25 04:54:41 PDT
This landed in r260585