[webkit-reviews] review granted: [Bug 227989] for-in should only emit one loop in bytecode : [Attachment 435019] Patch

Fri Aug 6 04:05:53 PDT 2021

Yusuke Suzuki <ysuzuki at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 227989: for-in should only emit one loop in bytecode
https://bugs.webkit.org/show_bug.cgi?id=227989

Attachment 435019: Patch

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

--- Comment #24 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 435019
  --> https://bugs.webkit.org/attachment.cgi?id=435019
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);