Bug 238143 – [JSC][ARMv7] Support proper near calls and JUMP_ISLANDS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geza Lore]

[JSC][ARMv7] Support proper near calls and JUMP_ISLANDS

[Geza Lore]

Created attachment 455247 [details]
Patch

[Geza Lore]

Created attachment 455248 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 455248 [details]
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.

[Geza Lore]

Comment on attachment 455248 [details]
Patch

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

>> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:911
>> +#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

relinkJump doesn't work  because on ARM jumps (i.e.: jit.jump()) are still far jumps, only jit.nearTailCall() are short branches. Will add relinkTailCall to other assemblers instead of the ifdef.

>> 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.

We no longer need this. The jump island in the last region is never used (even if we only have a single region). The allocator now understands this, so no need for this padding. Reservations smaller than (regionSize - islandRegionSize) will just have no islands (as we don't need them). All this is determined in the constructor of FixedVMPoolExecutableAllocator.

>> Source/WTF/wtf/EmbeddedFixedVector.h:52
>> +    }
> 
> 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

Renamed to createWithSizeAndConstructorArguments

>> Source/WTF/wtf/EmbeddedFixedVector.h:108
>> +
> 
> 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.

This constructor is private (used by createWithSizeAndConstructorArguments above), and I made the constructor of Base protected. This is the cleanest, alternative would be a new constructor of Trailing array that does no initialisation, but then half the constructors initialise inside the class, the other half does not, so it would be a mess and still have additional constructors. Given the constructors of TrailingArray are protected and of EmbeddedFixedVector are private, createWithSizeAndConstructorArguments is the only way of making one.

>> Source/WTF/wtf/FixedVector.h:79
>> +
> 
> Ditto.

Added static createWithSizeAndConstructorArguments factory insted.

>> Source/WTF/wtf/TrailingArray.h:71
>> +
> 
> Ditto.

This is needed by the EmbeddedFixedVector (which is a subclass), so see above. Made all constructors protected however.

[Geza Lore]

Created attachment 455907 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/90920556>

[Yusuke Suzuki]

Comment on attachment 455907 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:101
> +#elif CPU(ARM)

Use CPU(ARM_THUMB2)

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:117
> +#elif CPU(ARM)

Use CPU(ARM_THUMB2)

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:455
> +            m_numAllocators = (reservation.size + m_regionSize - 1) / m_regionSize;

m_numAllocators member is not necessary since m_allocators.size() can return the same value.

> Source/WTF/wtf/FixedVector.h:103
> +    FixedVector(std::unique_ptr<Storage> storage)

Let's take it std::unique_ptr<Storage>&&.

> Source/WTF/wtf/Vector.h:105
> +            new(NotNull, cur) T(args...);

Add space after `new`

new (NotNull, cur) T(args...);

[Geza Lore]

Created attachment 456900 [details]
review fixes

[EWS]

Committed r292540 (249378@main): <https://commits.webkit.org/249378@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456900 [details].