MIPS: Fix unaligned read/write operations in wasm.

MIPS: Fix unaligned read/write operations in wasm.

TEST=cctest/test-run-wasm/*, cctest/test-run-wasm-module/*, unittests
BUG=

Committed: https://crrev.com/1f5c91e4d810bc902f406158ceabd5f8f2a8ee86
Cr-Commit-Position: refs/heads/master@{#33678}

[akos.palfi.imgtec, 2016-01-19 15:56:35]

akos.palfi@imgtec.com changed reviewers:
+ alan.li@imgtec.com, balazs.kilvady@imgtec.com, mbrandy@us.ibm.com,
paul.lind@imgtec.com, titzer@chromium.org

[akos.palfi.imgtec, 2016-01-19 15:56:35]

PTAL.

@Matt: I know it overlaps with your CL, but there's a lot of test failures on
MIPS little-endian due to misalignment in wasm, so we would prefer to land this
ASAP and you could rebase your CL when it's done.

[akos.palfi.imgtec, 2016-01-19 16:00:45]

The CQ bit was checked by akos.palfi@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 16:00:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1581223002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1581223002/40001

[titzer, 2016-01-19 16:00:56]

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h#newcode1790
src/utils.h:1790: static inline void WriteUnalignedUInt32(void* p, uint32_t
value) {
Would it be enough to just do a memmove(p, &value, sizeof(uint32_t)) ?

[MTBrandyberry, 2016-01-19 16:01:41]

On 2016/01/19 15:56:35, akos.palfi.imgtec wrote:
> PTAL.
> 
> @Matt: I know it overlaps with your CL, but there's a lot of test failures on
> MIPS little-endian due to misalignment in wasm, so we would prefer to land
this
> ASAP and you could rebase your CL when it's done.

That's fine.  I've been distracted away from that piece of work.  I'll rebase
and revisit when I get the time.

[commit-bot: I haz the power, 2016-01-19 16:27:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-19 16:27:03]

Dry run: This issue passed the CQ dry run.

[ivica.bogosavljevic, 2016-01-20 10:32:57]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2016-01-20 10:32:58]

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h#newcode1790
src/utils.h:1790: static inline void WriteUnalignedUInt32(void* p, uint32_t
value) {
On 2016/01/19 16:00:55, titzer wrote:
> Would it be enough to just do a memmove(p, &value, sizeof(uint32_t)) ?

I would like to point out that the MIPS specific case should be possibly
extended to all architectures. 
Even though the code *(reinterpret_cast<uint32_t*>(p)) = value; works on other
architectures, accessing unaligned memory using wrong instructions can generate
Unaligned memory access on ARM which are then fixed in kernel, or is just slower
(X86-X64).

[akos.palfi.imgtec, 2016-01-21 20:10:30]

The CQ bit was checked by akos.palfi@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 20:10:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1581223002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1581223002/60001

[commit-bot: I haz the power, 2016-01-21 20:45:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 20:45:45]

Dry run: This issue passed the CQ dry run.

[akos.palfi.imgtec, 2016-01-21 21:00:22]

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h#newcode1790
src/utils.h:1790: static inline void WriteUnalignedUInt32(void* p, uint32_t
value) {
On 2016/01/19 16:00:55, titzer wrote:
> Would it be enough to just do a memmove(p, &value, sizeof(uint32_t)) ?

Thanks, great idea. I've removed the unions and have done some cleanup on the
way.

[akos.palfi.imgtec, 2016-01-21 21:07:35]

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1581223002/diff/40001/src/utils.h#newcode1790
src/utils.h:1790: static inline void WriteUnalignedUInt32(void* p, uint32_t
value) {
On 2016/01/20 10:32:57, ivica.bogosavljevic wrote:
> On 2016/01/19 16:00:55, titzer wrote:
> > Would it be enough to just do a memmove(p, &value, sizeof(uint32_t)) ?
> 
> I would like to point out that the MIPS specific case should be possibly
> extended to all architectures. 
> Even though the code *(reinterpret_cast<uint32_t*>(p)) = value; works on other
> architectures, accessing unaligned memory using wrong instructions can
generate
> Unaligned memory access on ARM which are then fixed in kernel, or is just
slower
> (X86-X64).

I think other archs support unaligned fixups in hardware and it should be
cheaper than doing a memmove. However, it would worth a deeper investigation. If
there's no objection, I'll leave the other archs as is, for now.

[paul.l..., 2016-01-22 01:21:52]

LGTM, very nice cleanup. Ben could you take another look?

[titzer, 2016-01-23 15:45:21]

https://codereview.chromium.org/1581223002/diff/60001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1581223002/diff/60001/src/utils.h#newcode1721
src/utils.h:1721: #if !(V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64)
Actually now that I think about it, a memmove() on all architectures is probably
going to generate exactly the same code (though I didn't check with LLVM),
except also handle alignment on ARM. Maybe we should have an #ifdef for
endianness, since I think we need to flip the byte order on big endian here as
well.

[titzer, 2016-01-28 18:58:48]

On 2016/01/23 15:45:21, titzer wrote:
> https://codereview.chromium.org/1581223002/diff/60001/src/utils.h
> File src/utils.h (right):
> 
> https://codereview.chromium.org/1581223002/diff/60001/src/utils.h#newcode1721
> src/utils.h:1721: #if !(V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64)
> Actually now that I think about it, a memmove() on all architectures is
probably
> going to generate exactly the same code (though I didn't check with LLVM),
> except also handle alignment on ARM. Maybe we should have an #ifdef for
> endianness, since I think we need to flip the byte order on big endian here as
> well.

FYI I have a CL that fixes these issues in:
https://codereview.chromium.org/1644023002/

[akos.palfi.imgtec, 2016-01-29 15:55:21]

On 2016/01/28 18:58:48, titzer wrote:
> On 2016/01/23 15:45:21, titzer wrote:
> > https://codereview.chromium.org/1581223002/diff/60001/src/utils.h
> > File src/utils.h (right):
> > 
> >
https://codereview.chromium.org/1581223002/diff/60001/src/utils.h#newcode1721
> > src/utils.h:1721: #if !(V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64)
> > Actually now that I think about it, a memmove() on all architectures is
> probably
> > going to generate exactly the same code (though I didn't check with LLVM),
> > except also handle alignment on ARM. Maybe we should have an #ifdef for
> > endianness, since I think we need to flip the byte order on big endian here
as
> > well.
> 
> FYI I have a CL that fixes these issues in:
> https://codereview.chromium.org/1644023002/

Thanks for your CL! I just tested it and it solves a lot of test failures on
MIPS, but a few test failure still exist. You can check them in our BB:
https://v8.mips.com/bbv8/builders/Target%20-%20edge/builds/6248

It seems we still have alignment issues in encoder.cc and the global_data
variable needs to be 8-byte aligned in wasm-run-utils.h. I've rebased my CL and
only kept the encoder.cc and wasm-run-utils.h changes.

Regarding the memmove vs. cast topic, on x64 the memmove() generates the exact
same code. I also tried to check on ARM, but I wasn't able to compile V8 for ARM
target with clang. If you could point me to a doc about the build process, I
would happily check it on ARM as well.

I also checked the memmove-version on a big-endian MIPS board and it works well,
we don't need separate #ifdef for big-endian, since the source and the target is
both big-endian.

[titzer, 2016-02-02 12:07:36]

On 2016/01/29 15:55:21, akos.palfi.imgtec wrote:
> On 2016/01/28 18:58:48, titzer wrote:
> > On 2016/01/23 15:45:21, titzer wrote:
> > > https://codereview.chromium.org/1581223002/diff/60001/src/utils.h
> > > File src/utils.h (right):
> > > 
> > >
> https://codereview.chromium.org/1581223002/diff/60001/src/utils.h#newcode1721
> > > src/utils.h:1721: #if !(V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64)
> > > Actually now that I think about it, a memmove() on all architectures is
> > probably
> > > going to generate exactly the same code (though I didn't check with LLVM),
> > > except also handle alignment on ARM. Maybe we should have an #ifdef for
> > > endianness, since I think we need to flip the byte order on big endian
here
> as
> > > well.
> > 
> > FYI I have a CL that fixes these issues in:
> > https://codereview.chromium.org/1644023002/
> 
> Thanks for your CL! I just tested it and it solves a lot of test failures on
> MIPS, but a few test failure still exist. You can check them in our BB:
> https://v8.mips.com/bbv8/builders/Target%20-%20edge/builds/6248
> 
> It seems we still have alignment issues in encoder.cc and the global_data
> variable needs to be 8-byte aligned in wasm-run-utils.h. I've rebased my CL
and
> only kept the encoder.cc and wasm-run-utils.h changes.
> 
> Regarding the memmove vs. cast topic, on x64 the memmove() generates the exact
> same code. I also tried to check on ARM, but I wasn't able to compile V8 for
ARM
> target with clang. If you could point me to a doc about the build process, I
> would happily check it on ARM as well.
> 
> I also checked the memmove-version on a big-endian MIPS board and it works
well,
> we don't need separate #ifdef for big-endian, since the source and the target
is
> both big-endian.

lgtm

[akos.palfi.imgtec, 2016-02-02 12:23:15]

The CQ bit was checked by akos.palfi@imgtec.com

[akos.palfi.imgtec, 2016-02-02 12:23:16]

The patchset sent to the CQ was uploaded after l-g-t-m from paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1581223002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-02-02 12:23:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1581223002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1581223002/80001

[commit-bot: I haz the power, 2016-02-02 12:45:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-02 12:45:48]

Description was changed from

==========
MIPS: Fix unaligned read/write operations in wasm.

TEST=cctest/test-run-wasm/*, cctest/test-run-wasm-module/*, unittests
BUG=
==========

to

==========
MIPS: Fix unaligned read/write operations in wasm.

TEST=cctest/test-run-wasm/*, cctest/test-run-wasm-module/*, unittests
BUG=

Committed: https://crrev.com/1f5c91e4d810bc902f406158ceabd5f8f2a8ee86
Cr-Commit-Position: refs/heads/master@{#33678}
==========

[commit-bot: I haz the power, 2016-02-02 12:45:49]

Patchset 5 (id:??) landed as
https://crrev.com/1f5c91e4d810bc902f406158ceabd5f8f2a8ee86
Cr-Commit-Position: refs/heads/master@{#33678}