Implement UnalignedLoad and UnalignedStore in WASM using LoadByte/Shift/Or and StoreByte/Shift/And.

Implement UnalignedLoad and UnalignedStore in WASM using
combination of LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=

Committed: https://crrev.com/ff2bd0fdcb8e4d61bd112b1965c781cf1ef210a8
Cr-Commit-Position: refs/heads/master@{#36422}

[ivica.bogosavljevic, 2016-05-04 11:04:40]

Description was changed from

==========
Implement UnalignedLoad and UnalignedStore in WASM using LoadByte/Shift/Or and
StoreByte/Shift/And.

Implement UnalignedLoad and UnalignedStore in WASM using
LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

to

==========
Implement UnalignedLoad and UnalignedStore in WASM using combination of
LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

[ivica.bogosavljevic, 2016-05-04 11:04:54]

Description was changed from

==========
Implement UnalignedLoad and UnalignedStore in WASM using combination of
LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

to

==========
Implement UnalignedLoad and UnalignedStore in WASM using
combination of LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

[ivica.bogosavljevic, 2016-05-04 11:15:18]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, balazs.kilvady@imgtec.com, gergely.kis@imgtec.com,
ilija.pavlovic.imgtec@gmail.com, marija.antic@imgtec.com,
miran.karic@imgtec.com, paul.lind@imgtec.com, titzer@chromium.org

[ivica.bogosavljevic, 2016-05-04 11:15:18]

PTAL.

This is an architecture independent part of supporting unaligned loads and
stores in WASM. This code was originally part of
https://codereview.chromium.org/1779713009, but it was moved to a separate
commit because fixing the original commit would require many reverts to the
patchsets.

The reduction of Load/Shift/Or and Store/Shift/Add that is used to emulate
unaligned access will be done in a separate commit, since it is architecture
dependent and not needed for those architectures that do support unaligned
access.

[titzer, 2016-05-06 16:06:47]

https://codereview.chromium.org/1928513002/diff/100001/src/compiler/machine-o...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1928513002/diff/100001/src/compiler/machine-o...
src/compiler/machine-operator.h:126: struct UnalignedAccessConfig {
Can we call this simply AlignmentRequirements?

[ivica.bogosavljevic, 2016-05-10 08:32:22]

PTAL

https://codereview.chromium.org/1928513002/diff/100001/src/compiler/machine-o...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1928513002/diff/100001/src/compiler/machine-o...
src/compiler/machine-operator.h:126: struct UnalignedAccessConfig {
On 2016/05/06 16:06:47, titzer wrote:
> Can we call this simply AlignmentRequirements?

Acknowledged.

[titzer, 2016-05-11 15:22:35]

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2606: auto determineType = [](bool signExtend,
uint32_t alignment) -> MachineType {
Can we lift this out to a helper method instead of a lambda?

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2737: if (!skip) {
Can you lift this up into the branch instead of using a boolean?

https://codereview.chromium.org/1928513002/diff/120001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/1928513002/diff/120001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:1307: TEST(Run_Wasm_LoadMemI32_alignment) {
Can you move this into test-run-load-store.cc?

[titzer, 2016-05-11 15:23:46]

titzer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[titzer, 2016-05-11 15:24:01]

+ FYI bmeurer

[Benedikt Meurer, 2016-05-12 04:12:13]

bmeurer@chromium.org changed reviewers:
+ jarin@chromium.org

[Benedikt Meurer, 2016-05-12 04:12:14]

+jarin

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:126: struct AlignmentRequirements {
Nit: class

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:181: const bool fullUnalignedAccessSupport_;
How about an enum here for this tristate logic? The two bools are very
confusing. I.e.

enum class UnalignedAccessSupport {
  kNoSupport,
  kSomeSupport,
  kFullSupport
};

[Benedikt Meurer, 2016-05-12 04:13:30]

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/instructi...
File src/compiler/instruction-selector.h (right):

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/instructi...
src/compiler/instruction-selector.h:142: static
MachineOperatorBuilder::AlignmentRequirements AlignmentConfiguration();
Nit: Rename to AlignmentRequirements

[Jarin, 2016-05-12 04:26:14]

I have just a bunch of nits (only looked at the compiler dir).

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
File src/compiler/machine-operator.cc (right):

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.cc:535: AlignmentRequirements alignmentConfig)
Style nit: alignmentConfig -> alignment_config (here and below)

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:129: bool noUnalignedAccessSupport)
Nit: Could you make the constructor private now that you have the static methods
for construction?

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:137: Vector<MachineType>
unalingedLoadSupportedTypes,
Typo: unalinged -> unaligned

https://codereview.chromium.org/1928513002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:164: bool IsUnalingedSupported(const
Vector<MachineType>& supported,
Typo: Unalinged -> Unaligned

[ivica.bogosavljevic, 2016-05-12 12:09:55]

https://codereview.chromium.org/1928513002/diff/120001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/1928513002/diff/120001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:1307: TEST(Run_Wasm_LoadMemI32_alignment) {
On 2016/05/11 15:22:35, titzer wrote:
> Can you move this into test-run-load-store.cc?

This test cannot be moved to test-run-load-store.cc, because tests in those
files test turbofan operators, whereas Unaligned access is generated by
wasm-compiler.cc, so it needs to be tested in this test suite.

We could copy code for unaligned access from wasm-compiler to a separate test in
test-run-load-store.cc and test it, but I don't like that approach. If somebody
breaks something in wasm-compiler related to unaligned access, the test will
pass, but wasm-compiler will be broken

[ivica.bogosavljevic, 2016-05-17 10:39:01]

A reminder.

[titzer, 2016-05-17 11:57:39]

Mostly looks good.

https://codereview.chromium.org/1928513002/diff/120001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/1928513002/diff/120001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:1307: TEST(Run_Wasm_LoadMemI32_alignment) {
On 2016/05/12 12:09:55, ivica.bogosavljevic wrote:
> On 2016/05/11 15:22:35, titzer wrote:
> > Can you move this into test-run-load-store.cc?
> 
> This test cannot be moved to test-run-load-store.cc, because tests in those
> files test turbofan operators, whereas Unaligned access is generated by
> wasm-compiler.cc, so it needs to be tested in this test suite.
> 
> We could copy code for unaligned access from wasm-compiler to a separate test
in
> test-run-load-store.cc and test it, but I don't like that approach. If
somebody
> breaks something in wasm-compiler related to unaligned access, the test will
> pass, but wasm-compiler will be broken

Acknowledged.

https://codereview.chromium.org/1928513002/diff/140001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1928513002/diff/140001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2621: int current) -> Node* {
Is there a way to refactor this to avoid a lambda? I would it more clear to have
a helper function in WasmCompiler rather than a lambda that closes over "this".

[ivica.bogosavljevic, 2016-05-17 14:22:08]

Addressed remarks. PTAL

@titzer: The next thing we would like to do is to add support for WASM on
big-endian. WASM has an assumption that memory is Little-endian ordered, for
this reason WASM is not working correctly on big-endian.

The idea on how to enable this is following: in memory, we keep data in
little-endian order even though we are on big-endian. We accomplish this by
modifying wasm-compiler.cc: before each data store, we change the data ordering
to little-endian. After each load, we change data ordering to big endian. By
doing this we introduce some overhead, but this needs to be done. It is possible
that we will need to introduce new turbofan operands for byte order reversing
(if not already there).

Please tell me what you think about this? Is this a reasonable approach? Is
there some reason why this would not work?

[ivica.bogosavljevic, 2016-05-19 14:23:49]

Reminder :)

[titzer, 2016-05-19 15:23:14]

Sorry for the delay. LGTM other than small naming nit.

https://codereview.chromium.org/1928513002/diff/160001/src/compiler/machine-o...
File src/compiler/machine-operator.cc (right):

https://codereview.chromium.org/1928513002/diff/160001/src/compiler/machine-o...
src/compiler/machine-operator.cc:535: AlignmentRequirements alignmentConfig)
s/alignmentConfig/alignmentRequirements/

https://codereview.chromium.org/1928513002/diff/160001/src/compiler/machine-o...
src/compiler/machine-operator.cc:539: alignment_config_(alignmentConfig) {
s/alignment_config/alignment_requirements/

[ivica.bogosavljevic, 2016-05-20 13:36:30]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 13:36:38]

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

[commit-bot: I haz the power, 2016-05-20 13:47:28]

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

[commit-bot: I haz the power, 2016-05-20 13:47:29]

Dry run: Try jobs failed on following builders:
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[ivica.bogosavljevic, 2016-05-20 14:23:25]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 14:23:31]

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

[commit-bot: I haz the power, 2016-05-20 14:30:02]

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

[commit-bot: I haz the power, 2016-05-20 14:30:03]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/7524)

[ivica.bogosavljevic, 2016-05-20 14:34:40]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 14:34:50]

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

[commit-bot: I haz the power, 2016-05-20 15:24:37]

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

[commit-bot: I haz the power, 2016-05-20 15:24:38]

Dry run: This issue passed the CQ dry run.

[ivica.bogosavljevic, 2016-05-20 15:58:43]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2016-05-20 15:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1928513002/#ps220001
(title: "Fix nits. Fix compilation error on Windows compiler")

[commit-bot: I haz the power, 2016-05-20 15:58:54]

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

[ivica.bogosavljevic, 2016-05-20 16:30:33]

The CQ bit was unchecked by ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2016-05-20 16:30:40]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

[commit-bot: I haz the power, 2016-05-20 16:30:52]

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

[ivica.bogosavljevic, 2016-05-20 17:22:28]

The CQ bit was unchecked by ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2016-05-20 17:22:35]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

[commit-bot: I haz the power, 2016-05-20 17:22:47]

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

[commit-bot: I haz the power, 2016-05-20 17:49:57]

Description was changed from

==========
Implement UnalignedLoad and UnalignedStore in WASM using
combination of LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

to

==========
Implement UnalignedLoad and UnalignedStore in WASM using
combination of LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

[commit-bot: I haz the power, 2016-05-20 17:49:58]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-20 17:51:05]

Description was changed from

==========
Implement UnalignedLoad and UnalignedStore in WASM using
combination of LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=
==========

to

==========
Implement UnalignedLoad and UnalignedStore in WASM using
combination of LoadByte/Shift/Or and StoreByte/Shift/And.

BUG=

Committed: https://crrev.com/ff2bd0fdcb8e4d61bd112b1965c781cf1ef210a8
Cr-Commit-Position: refs/heads/master@{#36422}
==========

[commit-bot: I haz the power, 2016-05-20 17:51:06]

Patchset 12 (id:??) landed as
https://crrev.com/ff2bd0fdcb8e4d61bd112b1965c781cf1ef210a8
Cr-Commit-Position: refs/heads/master@{#36422}