[webkit-reviews] review granted: [Bug 226107] Reduce Baseline JIT emitted code size for op_jfalse, op_jtrue, op_get_from_scope, op_resolve_scope. : [Attachment 429342] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 226107: Reduce Baseline JIT emitted code size for op_jfalse, op_jtrue,
op_get_from_scope, op_resolve_scope.
https://bugs.webkit.org/show_bug.cgi?id=226107

Attachment 429342: proposed patch.

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




--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 429342
  --> https://bugs.webkit.org/attachment.cgi?id=429342
proposed patch.

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

r=me

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1890
> +	   if constexpr (thunkNotUsedForResolveScope(resolveType)) { \
> +	       RELEASE_ASSERT_NOT_REACHED(); \
> +	       return { }; \
> +	   } \

Why not just RELEASE_ASSERT in generateOpResolveScopeThunk

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1910
> +#if CPU(X86_64)
> +    jit.push(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.pushPair(framePointerRegister, linkRegister);
> +#endif

not just emit a prologue?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1924
> +    jit.setupArguments<decltype(operationGetFromScope)>(globalObjectGPR,
instructionGPR);

operationResolveScopeForBaseline, not operationGetFromScope

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1933
> +#if CPU(X86_64)
> +    jit.pop(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.popPair(CCallHelpers::framePointerRegister,
CCallHelpers::linkRegister);
> +#endif

ditto for epilogue

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2353
> +    jit.move(metadataGPR, GPRInfo::numberTagRegister); // Preserve metadata
in a callee saved register.

I'd assert numberTagRegister is a CSR

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2360
> +    jit.move(TrustedImm64(JSValue::NumberTag), GPRInfo::numberTagRegister);

I'd do this before the exception check for sanity