Bug 155683 – [JSC] Put the x86 Assembler on a binary diet

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

[Benjamin Poulain]

[JSC] Put the x86 Assembler on a binary diet

[Benjamin Poulain]

Created attachment 274520 [details]
Patch

[Benjamin Poulain]

Created attachment 274523 [details]
Patch

[Benjamin Poulain]

Created attachment 274627 [details]
Patch

[Darin Adler]

Comment on attachment 274627 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:68
> +        This patch reduces the binary size by 66k. It is a small
> +        speedup on Sunspider.

Speed-up!? Wow.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:173
> +        // LocalWriter *CANNOT* be mixed with other types access to AssemblerBuffer.

"types access" -> "types of access"

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:174
> +        // AssemblerBuffer cannot be used until its LocalWritter goes out of scope.

"LocalWritter" -> "LocalWriter"

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:210
> +            char* m_storrageBuffer;

"m_storrageBuffer" -> "m_storageBuffer"

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:255
> +        NEVER_INLINE void outOfLineGrow()
> +        {
> +            m_storage.grow();
> +        }

Could make this private instead of protected.

> Source/JavaScriptCore/assembler/X86Assembler.h:2906
> +#if CPU(X86_64)
> +        // Byte operand register spl & above require a REX prefix (to prevent the 'H' registers be accessed).
> +        inline static bool byteRegRequiresRex(int reg)
> +        {
> +            static_assert(X86Registers::esp == 4, "Necessary condition for OR-masking");
> +            return (reg >= X86Registers::esp);
> +        }
> +        inline static bool byteRegRequiresRex(int a, int b)
> +        {
> +            return byteRegRequiresRex(a | b);
> +        }
> +
> +        // Registers r8 & above require a REX prefixe.
> +        inline static bool regRequiresRex(int reg)
> +        {
> +            static_assert(X86Registers::r8 == 8, "Necessary condition for OR-masking");
> +            return (reg >= X86Registers::r8);
> +        }
> +        inline static bool regRequiresRex(int a, int b)
> +        {
> +            return regRequiresRex(a | b);
> +        }
> +        inline static bool regRequiresRex(int a, int b, int c)
> +        {
> +            return regRequiresRex(a | b | c);
> +        }
> +#else
> +        inline static bool byteRegRequiresRex(int) { return false; }
> +        inline static bool byteRegRequiresRex(int, int) { return false; }
> +        inline static bool regRequiresRex(int) { return false; }
> +        inline static bool regRequiresRex(int, int) { return false; }
> +        inline static bool regRequiresRex(int, int, int) { return false; }
> +#endif

Since we are inside a class, all these "inline" can and should be omitted; they have no effect.

> Source/JavaScriptCore/assembler/X86Assembler.h:2919
> +            static const RegisterID noBase = X86Registers::ebp;
> +            static const RegisterID hasSib = X86Registers::esp;
> +            static const RegisterID noIndex = X86Registers::esp;

Can/should we use constexpr here instead of just const? Might be a good habit for the future.

[Benjamin Poulain]

Created attachment 274965 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 274965 [details]
Patch

Clearing flags on attachment: 274965

Committed r198708: <http://trac.webkit.org/changeset/198708>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.