Bug 227989 – for-in should only emit one loop in bytecode

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

for-in should only emit one loop in bytecode

[Keith Miller]

Created attachment 433583 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/80960811>

[Alexey Shvayka]

*** Bug 172458 has been marked as a duplicate of this bug. ***

[Keith Miller]

Created attachment 434250 [details]
WIP

[Keith Miller]

Created attachment 434311 [details]
test32bit

[Keith Miller]

Created attachment 434316 [details]
test32bit

[Keith Miller]

Created attachment 434437 [details]
test32bit

[Keith Miller]

Created attachment 434438 [details]
test32bit

[Keith Miller]

Created attachment 434439 [details]
test32bit

[Keith Miller]

Created attachment 434442 [details]
test32bit

[Keith Miller]

Created attachment 434444 [details]
test32bit

[Keith Miller]

Created attachment 434445 [details]
test32bit

[Keith Miller]

Created attachment 434446 [details]
test32bit

[Keith Miller]

Created attachment 434448 [details]
test32bit

[Keith Miller]

Created attachment 434449 [details]
test32bit

[Keith Miller]

Created attachment 434857 [details]
WIP

[Keith Miller]

Created attachment 435002 [details]
Patch

[Keith Miller]

Created attachment 435008 [details]
Patch

[Keith Miller]

Created attachment 435019 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 435019 [details]
Patch

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

Still reviewing.

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:-96
> -    AuxiliaryBarrier<WriteBarrier<JSString>*> m_propertyNames;
> -    WriteBarrier<StructureChain> m_prototypeChain;

Why is it moved? I think putting large fields first is the better ordering to prevent adding paddings.

[Yusuke Suzuki]

Comment on attachment 435019 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:-2170
> -                // FIXME: We can expand this for non x86 environments.
> -                // https://bugs.webkit.org/show_bug.cgi?id=134641
> -                if (!isX86())
> -                    return false;

Non-x86 architecture does not have TSO, so I think removing this is not correct since IIRC some code in this lambda relies on that.

[Keith Miller]

Comment on attachment 435019 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:-2170
>> -                    return false;
> 
> Non-x86 architecture does not have TSO, so I think removing this is not correct since IIRC some code in this lambda relies on that.

Yeah, I forgot why I removed this. I think I was thinking it was doing something else... I'll re-add it.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:-96
>> -    WriteBarrier<StructureChain> m_prototypeChain;
> 
> Why is it moved? I think putting large fields first is the better ordering to prevent adding paddings.

I'm not sure why I added it. I agree, I'll revert this relocation.

[Yusuke Suzuki]

Comment on attachment 435019 [details]
Patch

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

Still reviewing.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8405
> +            auto seenModes = metadata.m_enumeratorMetadata;

Lets' align it to the other like `auto seenModes = OptionSet<JSPropertyNameEnumerator::Mode>::fromRaw(metadata.m_enumeratorMetadata);`

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3017
> +        emitGetVirtualRegister(index, indexGPR);
> +        Jump notInit = branchTest32(Zero, modeGPR);
> +        // Need to use add64 since this is a JSValue int32.
> +        add64(TrustedImm32(1), indexGPR);

It is possible that indexGPR is double due to DFG (e.g. DFG OSR exits and it becomes double because DFG compiler thinks this is better to hold it double).
Can we have a check for that? (if it is double, let's go to the operation path).

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3064
> +    or8(regT2, AbsoluteAddress(&metadata.m_enumeratorMetadata));

Ditto about DFG double.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3078
> +    // Compute the offset.
> +    emitGetVirtualRegister(index, regT1);
> +    // If index is less than the enumerator's cached inline storage, then it's an inline access
> +    Jump outOfLineAccess = branch32(AboveOrEqual, regT1, Address(regT2, JSPropertyNameEnumerator::cachedInlineCapacityOffset()));

Ditto about DFG double.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3101
> +    emitGetVirtualRegister(index, regT1);

Why is it necessary? Soon after this, regT1 is overridden with `emitGetVirtualRegister(propertyName, regT1);`.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3140
> +    emitGetVirtualRegister(mode, regT0);
> +    or8(regT0, AbsoluteAddress(&metadata.m_enumeratorMetadata));

Ditto about DFG double

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:988
> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(modeRegister.jsValue().asUInt32());

It is possible that DFG makes it double, so let's use asUInt32AsAnyInt

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:989
> +    uint32_t index = indexRegister.jsValue().asUInt32();

Ditto.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1014
> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(GET(bytecode.m_mode).jsValue().asUInt32());

Ditto

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1018
> +    unsigned index = GET(bytecode.m_index).jsValue().asInt32();

Ditto.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1030
> +        } else

We do not need this else since the above branch finishes with RETURN_PROFILED.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1040
> +        // TODO Does this actually ever throw? Isn't string backed by a UUID?

Maybe, changing it to FIXME?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1058
> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(GET(bytecode.m_mode).jsValue().asUInt32());

Ditto.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1068
> +            RETURN(jsBoolean(base->hasProperty(globalObject, GET(bytecode.m_index).jsValue().asUInt32())));

Ditto about asUInt32.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1081
> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(GET(bytecode.m_mode).jsValue().asUInt32());

Ditto

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1090
> +            RETURN(jsBoolean(base->hasOwnProperty(globalObject, GET(bytecode.m_index).jsValue().asUInt32())));

Ditto.

[Yusuke Suzuki]

Comment on attachment 435019 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGClobberize.h:343
> +    case EnumeratorNextUpdatePropertyName:

To define PureValue, you need to pass valid OpInfos too. In this node's case, seenModes info is missing in PureValue.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:-1124
> -                        

Can we share the code with setJSArraySaneChainIfPossible?
It looks like this is very similar and having the exact same long comment.

> Source/JavaScriptCore/dfg/DFGNode.h:3273
> +    EnumeratorMetadata enumeratorMetadata()
> +    {
> +        return m_opInfo2.as<EnumeratorMetadata>();
> +    }

How about returning OptionSet from this function?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2481
> +    JSValue result = bitwise_cast<JSValue>(static_cast<uint64_t>(mode) << 32 | index | JSValue::NumberTag);

This is making broken JSInt32 (this pattern never appears in the valid number). I think we should mimic double's pattern instead.
For example, we can make it DoubleEncodeOffset | static_cast<uint64_t>(mode) << 32 | index. This is better since this is still a valid JSValue double's bit pattern.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2483
> +    JSValue result = JSValue(mode, index);

I think we can do the same thing for 32bit too: I mean, let's just encode mode and index into double value bit pattern.
In 32bit case, we are not using offset. So, `uint64_t value = static_cast<uint64_t>(mode) << 32 | index` can be a valid JSValue double pattern in 32bit, correct?
So, we can make DFG AI valid.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2517
> +    scope.assertNoException();

RETURN_IF_EXCEPTION(scope, { }); is better since there is terminated exception.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2534
> +        return JSValue::encode(jsBoolean(base.getObject()->hasProperty(globalObject, index)));

Use `jsCast<JSObject*>` instead of getObject since we already checked this is an object.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2550
> +        return JSValue::encode(jsBoolean(base.getObject()->hasOwnProperty(globalObject, index)));

Use `jsCast<JSObject*>` instead of getObject since we already checked this is an object.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:473
>              Edge child1 = m_graph.child(node, 0);

This clause does not include `EnumeratorGetByVal` while we are checking `node->op() == EnumeratorGetByVal`. Is it missing?

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1233
> +            // This can actually return a JS null but we branch of that case in the same basic block so we don't want to mess with
> +            // fixup's logic.

I think we can make valid JSValue double bit pattern anytime, so we do not need to have this comment.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2669
> +        else

Add ASSERT for else case with expected format type.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13490
> +#if USE(JSVALUE64)
> +        m_jit.move(TrustedImm64(JSValue::NumberTag | static_cast<uint64_t>(JSPropertyNameEnumerator::IndexedMode) << 32), resultRegs.payloadGPR());
> +        m_jit.or64(scratch.gpr(), resultRegs.payloadGPR());
> +#else
> +        m_jit.move(TrustedImm32(JSPropertyNameEnumerator::IndexedMode), resultRegs.tagGPR());
> +#endif

Ditto. We should make it valid double bit pattern.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13519
> +#if USE(JSVALUE64)
> +        m_jit.or64(TrustedImm64(JSValue::NumberTag | static_cast<uint64_t>(JSPropertyNameEnumerator::OwnStructureMode) << 32), resultRegs.payloadGPR());
> +#else
> +        m_jit.move(TrustedImm32(JSPropertyNameEnumerator::OwnStructureMode), resultRegs.tagGPR());
> +#endif

Ditto. We should make it valid double bit pattern.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13560
> +    m_jit.rshift64(pairGPR, TrustedImm32(32), resultGPR);

We have double-offset (or NumberTag in the current code). In ARM64, we can use bit-extract to get appropriate bits easily.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13063
> +            m_out.appendTo(increment);

Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13096
> +            m_out.appendTo(increment);

Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13102
> +            setJSValue(m_out.bitOr(m_out.zeroExt(index, Int64), m_out.constInt64(JSValue::NumberTag | static_cast<uint64_t>(JSPropertyNameEnumerator::OwnStructureMode) << 32)));

Should we remove some bits from NumberTag here? If we use full NumberTag, the generated bit pattern becomes broken Int32 (this bit pattern never appears in the usual JSValue). I think we should make it look like some sort of Encoded Double.
Like adding double-offset instead.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13169
> +                cachedNameResult = m_out.anchor(m_out.zeroExtPtr(m_out.loadPtr(m_out.baseIndex(m_heaps.WriteBarrierBuffer_bufferContents.atAnyIndex(), namesVector, m_out.zeroExt(index, Int64), ScalePtr))));

m_out.zeroExtPtr is not necessary since this is the result of loadPtr.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13185
> +            m_out.appendTo(continuation);

Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13189
> +        LValue result = m_out.phi(Int64);
> +        ASSERT(outOfBoundsResult || cachedNameResult || operationResult);
> +        m_out.addIncomingToPhiIfSet(result, outOfBoundsResult, cachedNameResult, operationResult);

Why not using existing Vector<ValueFromBlock, N> results, and m_out.phi(Int64, results)?
We can just append LValue only when it exists. And then we can pass this vector to `m_out.phi`. So we do not need to check whether each ValueFromBlock is nullptr or not.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13223
> +        m_out.appendTo(checkIsCellBlock);

Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13294
> +        LValue result = m_out.phi(Int64, genericICResult, outOfLineResult, inlineResult);
> +        if (badStructureSlowPathResult)
> +            m_out.addIncomingToPhi(result, badStructureSlowPathResult);
> +        setJSValue(result);

Ditto. Why not using existing Vector<ValueFromBlock, N> results, and m_out.phi(Int64, results)?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13315
> +        m_out.appendTo(isNamedBlock);

Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1029
> +            RETURN_PROFILED(baseValue.getObject()->getDirect(index < enumerator->cachedInlineCapacity() ? index : index - enumerator->cachedInlineCapacity() + firstOutOfLineOffset));

Let's just use `jsCast<JSObject*>` instead of getObject() since we know that this is JSObject*.

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:177
> +        scope.assertNoException();

Since there is termination exception, I suggest just using RETURN_IF_EXCEPTION(scope, nullptr);

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:202
> +            scope.assertNoException();

Since there is termination exception, I suggest just using RETURN_IF_EXCEPTION(scope, nullptr);

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:209
> +        scope.assertNoException();

Since there is termination exception, I suggest just using RETURN_IF_EXCEPTION(scope, nullptr);

[Keith Miller]

Comment on attachment 435019 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8405
>> +            auto seenModes = metadata.m_enumeratorMetadata;
> 
> Lets' align it to the other like `auto seenModes = OptionSet<JSPropertyNameEnumerator::Mode>::fromRaw(metadata.m_enumeratorMetadata);`

Done.

>> Source/JavaScriptCore/dfg/DFGNode.h:3273
>> +    }
> 
> How about returning OptionSet from this function?

Nice! done.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2481
>> +    JSValue result = bitwise_cast<JSValue>(static_cast<uint64_t>(mode) << 32 | index | JSValue::NumberTag);
> 
> This is making broken JSInt32 (this pattern never appears in the valid number). I think we should mimic double's pattern instead.
> For example, we can make it DoubleEncodeOffset | static_cast<uint64_t>(mode) << 32 | index. This is better since this is still a valid JSValue double's bit pattern.

Yeah, I think you're right. I'll switch it to `DoubleEncodeOffset | static_cast<uint64_t>(mode) << 32 | index`

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2483
>> +    JSValue result = JSValue(mode, index);
> 
> I think we can do the same thing for 32bit too: I mean, let's just encode mode and index into double value bit pattern.
> In 32bit case, we are not using offset. So, `uint64_t value = static_cast<uint64_t>(mode) << 32 | index` can be a valid JSValue double pattern in 32bit, correct?
> So, we can make DFG AI valid.

I think this is a double in 32-bit though? It's in double form as long as the tag is below the last explicit tag (-8 I believe).

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2485
> +    ASSERT(result.isNumber());

I'll change this assert to `isDouble()` too.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2517
>> +    scope.assertNoException();
> 
> RETURN_IF_EXCEPTION(scope, { }); is better since there is terminated exception.

What's the value in having assertNoException then? Seems like it's mostly pointless and will probably lead to bugs.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2534
>> +        return JSValue::encode(jsBoolean(base.getObject()->hasProperty(globalObject, index)));
> 
> Use `jsCast<JSObject*>` instead of getObject since we already checked this is an object.

Seems like the extra check should get eliminated but I'll make the change.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2550
>> +        return JSValue::encode(jsBoolean(base.getObject()->hasOwnProperty(globalObject, index)));
> 
> Use `jsCast<JSObject*>` instead of getObject since we already checked this is an object.

Ditto.

>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:473
>>              Edge child1 = m_graph.child(node, 0);
> 
> This clause does not include `EnumeratorGetByVal` while we are checking `node->op() == EnumeratorGetByVal`. Is it missing?

Ah, yeah, that's leftover from when I was implementing EnumeratorGetByVal by just copying the logic of GetByVal. But EnumeratorGetByVal accesses non-indexed properties so you can't infer anything about what the produced value will be based on the indexing type, like we can for GetByVal. So I removed most of the code but I guess I left that check behind.

Will remove.

>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1233
>> +            // fixup's logic.
> 
> I think we can make valid JSValue double bit pattern anytime, so we do not need to have this comment.

Yeah, that's a stale comment. Will remove.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1239
> +            // FIXME: We should have a value profile whether we've ever actually seen a symbol/string.

This is also stale, removing.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2669
>> +        else
> 
> Add ASSERT for else case with expected format type.

Done.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13490
>> +#endif
> 
> Ditto. We should make it valid double bit pattern.

Done.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13519
>> +#endif
> 
> Ditto. We should make it valid double bit pattern.

Done.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13560
>> +    m_jit.rshift64(pairGPR, TrustedImm32(32), resultGPR);
> 
> We have double-offset (or NumberTag in the current code). In ARM64, we can use bit-extract to get appropriate bits easily.

I'll add the ARM64 flavor. I'm not sure what you mean by the NumberTag comment though.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13063
>> +            m_out.appendTo(increment);
> 
> Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

Nope, lastNext will always be preserved as long as no-one overwrites it via the 2 argument `appendTo`. i.e. as long as any of these functions never calls the 2 argument `appendTo` last next should be preserved.  

FWIW, I never really understood why so much code here uses the lastNext idiom, I personally find it very confusing.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13096
>> +            m_out.appendTo(increment);
> 
> Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

Ditto.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13102
>> +            setJSValue(m_out.bitOr(m_out.zeroExt(index, Int64), m_out.constInt64(JSValue::NumberTag | static_cast<uint64_t>(JSPropertyNameEnumerator::OwnStructureMode) << 32)));
> 
> Should we remove some bits from NumberTag here? If we use full NumberTag, the generated bit pattern becomes broken Int32 (this bit pattern never appears in the usual JSValue). I think we should make it look like some sort of Encoded Double.
> Like adding double-offset instead.

Done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13169
>> +                cachedNameResult = m_out.anchor(m_out.zeroExtPtr(m_out.loadPtr(m_out.baseIndex(m_heaps.WriteBarrierBuffer_bufferContents.atAnyIndex(), namesVector, m_out.zeroExt(index, Int64), ScalePtr))));
> 
> m_out.zeroExtPtr is not necessary since this is the result of loadPtr.

I added it to help with any future arm64_32 FTL.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13185
>> +            m_out.appendTo(continuation);
> 
> Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

Ditto.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13189
>> +        m_out.addIncomingToPhiIfSet(result, outOfBoundsResult, cachedNameResult, operationResult);
> 
> Why not using existing Vector<ValueFromBlock, N> results, and m_out.phi(Int64, results)?
> We can just append LValue only when it exists. And then we can pass this vector to `m_out.phi`. So we do not need to check whether each ValueFromBlock is nullptr or not.

That's a good idea! Changed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13223
>> +        m_out.appendTo(checkIsCellBlock);
> 
> Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

Ditto.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13294
>> +        setJSValue(result);
> 
> Ditto. Why not using existing Vector<ValueFromBlock, N> results, and m_out.phi(Int64, results)?

Good idea. done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13315
>> +        m_out.appendTo(isNamedBlock);
> 
> Don't we need to get lastNext here and append it later as m_out.appendTo(continuation, lastNext) ?

Ditto.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3017
>> +        add64(TrustedImm32(1), indexGPR);
> 
> It is possible that indexGPR is double due to DFG (e.g. DFG OSR exits and it becomes double because DFG compiler thinks this is better to hold it double).
> Can we have a check for that? (if it is double, let's go to the operation path).

Per, discussion offline. Since we know the variable associated with the index will only have one SetLocal in DFG and that SetLocal will always see a `SpecInt32Any` we will always OSR exit with an int32 form.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3064
>> +    or8(regT2, AbsoluteAddress(&metadata.m_enumeratorMetadata));
> 
> Ditto about DFG double.

Ditto.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3078
>> +    Jump outOfLineAccess = branch32(AboveOrEqual, regT1, Address(regT2, JSPropertyNameEnumerator::cachedInlineCapacityOffset()));
> 
> Ditto about DFG double.

Ditto.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3101
>> +    emitGetVirtualRegister(index, regT1);
> 
> Why is it necessary? Soon after this, regT1 is overridden with `emitGetVirtualRegister(propertyName, regT1);`.

Oh, the emitGetVirtualRegister(propertyName, regT1); should be right after the structureMismatch link above. But I actually think we can load the index into regT3 and not touch propertyName/regT1.

The point of this load is to provide the IC the int32 value rather than the propertyName JSString for IndexedMode. That should be faster, at least as long as there's not indexed getters or something weird.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3140
>> +    or8(regT0, AbsoluteAddress(&metadata.m_enumeratorMetadata));
> 
> Ditto about DFG double

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:988
>> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(modeRegister.jsValue().asUInt32());
> 
> It is possible that DFG makes it double, so let's use asUInt32AsAnyInt

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:989
>> +    uint32_t index = indexRegister.jsValue().asUInt32();
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1014
>> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(GET(bytecode.m_mode).jsValue().asUInt32());
> 
> Ditto

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1018
>> +    unsigned index = GET(bytecode.m_index).jsValue().asInt32();
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1029
>> +            RETURN_PROFILED(baseValue.getObject()->getDirect(index < enumerator->cachedInlineCapacity() ? index : index - enumerator->cachedInlineCapacity() + firstOutOfLineOffset));
> 
> Let's just use `jsCast<JSObject*>` instead of getObject() since we know that this is JSObject*.

Ditto. Made the change anyway.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1030
>> +        } else
> 
> We do not need this else since the above branch finishes with RETURN_PROFILED.

True fixed.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1040
>> +        // TODO Does this actually ever throw? Isn't string backed by a UUID?
> 
> Maybe, changing it to FIXME?

Per the discussion on TerminationException we should keep the CHECK_EXCEPTION() and I'll delete this comment.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1058
>> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(GET(bytecode.m_mode).jsValue().asUInt32());
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1068
>> +            RETURN(jsBoolean(base->hasProperty(globalObject, GET(bytecode.m_index).jsValue().asUInt32())));
> 
> Ditto about asUInt32.

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1081
>> +    auto mode = static_cast<JSPropertyNameEnumerator::Mode>(GET(bytecode.m_mode).jsValue().asUInt32());
> 
> Ditto

Ditto.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1090
>> +            RETURN(jsBoolean(base->hasOwnProperty(globalObject, GET(bytecode.m_index).jsValue().asUInt32())));
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:177
>> +        scope.assertNoException();
> 
> Since there is termination exception, I suggest just using RETURN_IF_EXCEPTION(scope, nullptr);

I don't think we can have a TerminationException here. hasEnumerableProperty returns false if any exception is thrown.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:202
>> +            scope.assertNoException();
> 
> Since there is termination exception, I suggest just using RETURN_IF_EXCEPTION(scope, nullptr);

Yeah, I guess that's fair, I'll change this code.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:209
>> +        scope.assertNoException();
> 
> Since there is termination exception, I suggest just using RETURN_IF_EXCEPTION(scope, nullptr);

I don't think we can have a TerminationException here. hasEnumerableProperty returns false if any exception is thrown.

[Keith Miller]

Created attachment 435135 [details]
Patch for landing

[EWS]

Committed r280760 (240345@main): <https://commits.webkit.org/240345@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435135 [details].