[Webkit-unassigned] [Bug 238143] [JSC][ARMv7] Support proper near calls and JUMP_ISLANDS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=238143

Yusuke Suzuki <ysuzuki at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ysuzuki at apple.com

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

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

> Source/JavaScriptCore/ChangeLog:32
> +        Also removedetunused repatchCompact methods from assemblers.

typo: removedetunused

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:911
> +#if CPU(ARM)
> +            AssemblerType::relinkTailCall(nearCall.dataLocation(), destination.untaggedExecutableAddress());
> +#else
>              AssemblerType::relinkJump(nearCall.dataLocation(), destination.dataLocation());
> +#endif

We need to avoid these if-defs as much as possible. Why doesn't relinkJump work?
If we need special handling for tail-call instead, then, we should define relinkTailCall for all assemblers.
Anyway, let's not add ifdef in AbstractMacroAssembler.h

> Source/JavaScriptCore/bytecode/Repatch.cpp:1885
> +    for (CallToCodePtr callToCodePtr : calls)
>          patchBuffer.link(callToCodePtr.call, FunctionPtr<JSEntryPtrTag>(callToCodePtr.codePtr));

Nice

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:-356
> -#endif // ENABLE(JUMP_ISLANDS)

Why is it removed? This is necessary to protect it against very small reservation size.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:717
> +                auto jump = jit.nearTailCall();

Let's rename this variable to nearTailCall.

> Source/WTF/wtf/EmbeddedFixedVector.h:52
> +    template<typename... Args>
> +    static UniqueRef<EmbeddedFixedVector> create(unsigned size, Args&&... args)
> +    {
> +        return UniqueRef { *new (NotNull, fastMalloc(Base::allocationSize(size))) EmbeddedFixedVector(size, std::forward<Args>(args)...) };
> +    }

We should have much more descriptive name for this create.
It is hard to know what this create factory function does. For example,

T::create(3, 1, 2, 3);

Then, we do not know whether
1. we are creating 3 elements array where each element is constructed with 1, 2, 3.
2. we are creating 4 elements array, 3, 1, 2, 3

> Source/WTF/wtf/EmbeddedFixedVector.h:108
> +    template<typename... Args>
> +    explicit EmbeddedFixedVector(unsigned size, Args&&... args)
> +        : Base(size, std::forward<Args>(args)...)
> +    {
> +    }
> +

We should not expose this constructor, and we should define a factory function which has much more descriptive name T::createXXX.
The rationale is the same to the above one.

> Source/WTF/wtf/FixedVector.h:79
> +    template<typename... Args>
> +    explicit FixedVector(size_t size, Args&&... args)
> +        : m_storage(size ? Storage::create(size, std::forward<Args>(args)...).moveToUniquePtr() : nullptr)
> +    { }
> +

Ditto.

> Source/WTF/wtf/PlatformEnable.h:615
> +#if (CPU(ARM64) && CPU(ADDRESS64)) || CPU(ARM)

Use CPU(ARM_THUMB2)

> Source/WTF/wtf/TrailingArray.h:71
> +    template<typename... Args>
> +    TrailingArray(unsigned size, Args&&... args)
> +        : m_size(size)
> +    {
> +        static_assert(std::is_final_v<Derived>);
> +        VectorTypeOperations<T>::initializeWithArgs(begin(), end(), std::forward<Args>(args)...);
> +    }
> +

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220326/66ac2393/attachment-0001.htm>