[webkit-reviews] review granted: [Bug 184702] Templatize CodePtr/Refs/FunctionPtrs with PtrTags. : [Attachment 338152] proposed patch rebased.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 17 15:13:33 PDT 2018


Filip Pizlo <fpizlo at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 184702: Templatize CodePtr/Refs/FunctionPtrs with PtrTags.
https://bugs.webkit.org/show_bug.cgi?id=184702

Attachment 338152: proposed patch rebased.

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




--- Comment #5 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 338152
  --> https://bugs.webkit.org/attachment.cgi?id=338152
proposed patch rebased.

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

r=me.  The suggestions about using alternate tags can be addressed in another
bug.

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:332
> +    CodeLocationLabel<JSEntryPtrTag> m_callReturnLocationOrPatchableJump;
> +    CodeLocationLabel<JSEntryPtrTag> m_hotPathBeginOrSlowPathStart;
> +    CodeLocationNearCall<JSEntryPtrTag> m_hotPathOther;

I think these should be JSInternalPtrTag, since these are not being used as
targets for a JS call.	They are being used to point at some patchable stuff in
the call IC.

> Source/JavaScriptCore/ftl/FTLLazySlowPath.h:88
> +    CodeLocationLabel<JSEntryPtrTag> m_done;

I think this should be JSInternalPtrTag.  It's not meant to point to the call
entrypoint of anything.

> Source/JavaScriptCore/jit/JITMathIC.h:67
> +    CodeLocationLabel<JSEntryPtrTag> doneLocation() { return
m_inlineStart.labelAtOffset(m_inlineSize); }
> +    CodeLocationLabel<JSEntryPtrTag> slowPathStartLocation() { return
m_inlineStart.labelAtOffset(m_deltaFromStartToSlowPathStart); }
> +    CodeLocationCall<JSEntryPtrTag> slowPathCallLocation() { return
m_inlineStart.callAtOffset(m_deltaFromStartToSlowPathCallLocation); }

I think these should be JSInternal, since none of these are meant to call to
the JS call entrypoint of any function.

> Source/JavaScriptCore/jit/JITMathIC.h:250
> +    CodeLocationLabel<JSEntryPtrTag> m_inlineStart;

ISInternal, because this does not for doing JS calls

> Source/JavaScriptCore/llint/LLIntData.cpp:78
> +	   JSEntryPtrTag, // llint_program_prologue
> +	   JSEntryPtrTag, // llint_eval_prologue
> +	   JSEntryPtrTag, // llint_module_program_prologue
> +	   JSEntryPtrTag, // llint_function_for_call_prologue
> +	   JSEntryPtrTag, // llint_function_for_construct_prologue
> +	   JSEntryPtrTag, // llint_function_for_call_arity_check
> +	   JSEntryPtrTag, // llint_function_for_construct_arity_check
> +	   JSEntryPtrTag, // llint_generic_return_point
>	   BytecodePtrTag, // llint_throw_from_slow_path_trampoline
>	   ExceptionHandlerPtrTag, // llint_throw_during_call_trampoline
> -	   CodePtrTag, // llint_native_call_trampoline
> -	   CodePtrTag, // llint_native_construct_trampoline
> -	   CodePtrTag, // llint_internal_function_call_trampoline
> -	   CodePtrTag, // llint_internal_function_construct_trampoline
> +	   JSEntryPtrTag, // llint_native_call_trampoline
> +	   JSEntryPtrTag, // llint_native_construct_trampoline
> +	   JSEntryPtrTag, // llint_internal_function_call_trampoline
> +	   JSEntryPtrTag, // llint_internal_function_construct_trampoline

It would be cool if we could give these JSInternal since these aren't meant to
be used as targets of JS calls.


More information about the webkit-reviews mailing list