Bug 155683 - [JSC] Put the x86 Assembler on a binary diet
Summary: [JSC] Put the x86 Assembler on a binary diet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-19 12:59 PDT by Benjamin Poulain
Modified: 2016-03-25 20:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (42.65 KB, patch)
2016-03-19 13:29 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (42.72 KB, patch)
2016-03-19 14:27 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (43.47 KB, patch)
2016-03-21 15:31 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (43.42 KB, patch)
2016-03-25 19:01 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2016-03-19 12:59:42 PDT
[JSC] Put the x86 Assembler on a binary diet
Comment 1 Benjamin Poulain 2016-03-19 13:29:02 PDT
Created attachment 274520 [details]
Patch
Comment 2 Benjamin Poulain 2016-03-19 14:27:15 PDT
Created attachment 274523 [details]
Patch
Comment 3 Benjamin Poulain 2016-03-21 15:31:52 PDT
Created attachment 274627 [details]
Patch
Comment 4 Darin Adler 2016-03-24 20:26:16 PDT
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.
Comment 5 Benjamin Poulain 2016-03-25 19:01:41 PDT
Created attachment 274965 [details]
Patch
Comment 6 WebKit Commit Bot 2016-03-25 20:47:24 PDT
Comment on attachment 274965 [details]
Patch

Clearing flags on attachment: 274965

Committed r198708: <http://trac.webkit.org/changeset/198708>
Comment 7 WebKit Commit Bot 2016-03-25 20:47:28 PDT
All reviewed patches have been landed.  Closing bug.