1459850 - Wasm baseline: BaseCompiler::load() for 8-bit loads on x86: remove use of scratch reg

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Julian Seward [:jseward]]

For x86(32), BaseCompiler::load() loads indirectly via the scratch reg
(ebx) when the data size is 8 bits and the eventual destination is esi
or edi.  This is pointless because the actual load created by 
|masm.wasmLoad(*access, srcAddr, out);| is a mov{s,z}bl instruction and
so is unconstrained as to the output register.  (a.k.a. the same logic
that applies for byte stores doesn't apply here.)

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1459850-8bitloads-1.diff

Comment 2

[Julian Seward [:jseward]]

For the (admittedly artificial) following test case, the before/after code
is as follows:

                    `(module
                        (memory (export "memory") 1 1)
                        (func (export "testfn") (param i32) (result i32)
                           (get_local 0)
                           i32.const 0x80
                           i32.load8_u

                           i32.const 0x81
                           i32.load8_s

                           i32.const 0x82
                           i32.load8_u

                           i32.const 0x83
                           i32.load8_s

                           i32.const 0x84
                           i32.load8_u

                           drop
                           drop
                           drop
                           drop
                           drop
                        )
                     )`

Before                                     |After
                                           |
 movl       $0x80, %eax                    | movl       $0x80, %eax
 movl       0x4(%esp), %ecx                | movl       0x4(%esp), %ecx
 addl       0x0(%ecx), %eax                | addl       0x0(%ecx), %eax
 movzbl     0x0(%eax), %eax                | movzbl     0x0(%eax), %eax
 movl       $0x81, %ecx                    | movl       $0x81, %ecx
 movl       0x4(%esp), %edx                | movl       0x4(%esp), %edx
 addl       0x0(%edx), %ecx                | addl       0x0(%edx), %ecx
 movsbl     0x0(%ecx), %ecx                | movsbl     0x0(%ecx), %ecx
 movl       $0x82, %edx                    | movl       $0x82, %edx
 movl       0x4(%esp), %esi                | movl       0x4(%esp), %esi
 addl       0x0(%esi), %edx                | addl       0x0(%esi), %edx
 movzbl     0x0(%edx), %edx                | movzbl     0x0(%edx), %edx
 movl       $0x83, %esi                    | movl       $0x83, %esi
 movl       0x4(%esp), %edi                | movl       0x4(%esp), %edi
 addl       0x0(%edi), %esi                | addl       0x0(%edi), %esi
 movsbl     0x0(%esi), %ebx                | movsbl     0x0(%esi), %esi <---
 movl       %ebx, %esi                     |
 movl       $0x84, %edi                    | movl       $0x84, %edi
 movl       0xc(%esp), %ebx                | movl       0xc(%esp), %ebx
 push       %ebx                           | push       %ebx
 push       %eax                           | push       %eax
 push       %ecx                           | push       %ecx
 push       %edx                           | push       %edx
 push       %esi                           | push       %esi
 movl       0x18(%esp), %eax               | movl       0x18(%esp), %eax
 addl       0x0(%eax), %edi                | addl       0x0(%eax), %edi
 movzbl     0x0(%edi), %ebx                | movzbl     0x0(%edi), %edi <---
 movl       %ebx, %edi                     |
 addl       $4, %esp                       | addl       $4, %esp
 addl       $4, %esp                       | addl       $4, %esp
 addl       $4, %esp                       | addl       $4, %esp
 addl       $4, %esp                       | addl       $4, %esp
 pop        %eax                           | pop        %eax

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 8973953 [details] [diff] [review]
bug1459850-8bitloads-1.diff

Review of attachment 8973953 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.  This code probably predates the use of wasmLoad, and might actually be quite old.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d050e53f0d3
Wasm baseline: BaseCompiler::load() for 8-bit loads on x86: remove use of scratch reg.  r=lth.

Comment 5

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1d050e53f0d3