[webkit-reviews] review granted: [Bug 214737] for..of intrinsics implementation for 32bits : [Attachment 405291] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Paulo Matos <pmatos at igalia.com>'s
request for review:
Bug 214737: for..of intrinsics implementation for 32bits
https://bugs.webkit.org/show_bug.cgi?id=214737

Attachment 405291: Patch

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




--- Comment #12 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 405291
  --> https://bugs.webkit.org/attachment.cgi?id=405291
Patch

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

r=me with comments

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:714
> +    void store8(RegisterID src, const ArmAddress address)

We do not need to have `const`.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:908
> +    void store8(RegisterID src, const Address address)

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:913
> +    void store8(RegisterID src, const BaseIndex address)

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:938
> +    void store8(const RegisterID src, const RegisterID addrreg)

Ditto.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:633
> +		       case Constant:
> +			   sideState->tmps[i] = recovery.constant();
> +			   break;

This is the same implementation. Put it outside of ifdef.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:639
> +		       case UnboxedInt32InGPR:
> +		       case Int32DisplacedInJSStack: {
> +			   sideState->tmps[i] =
jsNumber(static_cast<int32_t>(tmpScratch[i + tmpOffset]));
> +			   break;
> +		       }

Ditto.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:644
> +		       case InPair:
> +		       case DisplacedInJSStack: {
>			   sideState->tmps[i] =
reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset];
> -#else
> +			   break;

Ditto.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:652
> +		       case CellDisplacedInJSStack:
> +		       case UnboxedCellInGPR: {
>			   EncodedValueDescriptor* valueDescriptor =
bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset);
>			   sideState->tmps[i] = JSValue(JSValue::CellTag,
valueDescriptor->asBits.payload);
> -#endif
>			   break;
>		       }

Ditto.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:654
> +		       case BooleanDisplacedInJSStack:

Why?

> Source/JavaScriptCore/jit/JITCall32_64.cpp:392
> +    VirtualRegister vr = destinationFor(bytecode,
m_bytecodeIndex.checkpoint()).virtualRegister();

Remove it.

> Source/JavaScriptCore/jit/JITCall32_64.cpp:398
> +    emitJumpSlowCaseIfNotJSCell(vr, regT1);

Let's use `emitJumpSlowCaseIfNotJSCell(regT1)`.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2773
> +    bieq t1, UndefinedTag, .iteratorNextGeneric
> +    btinz t0, .iteratorNextGeneric

I think this is not correct. We should have JSEmpty check here.