[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=

Committed: https://crrev.com/77c9cb8341e85e8e2f1b65d5004ac90e7dc060ea
Cr-Commit-Position: refs/heads/master@{#38183}

[john.yan, 2016-06-07 14:25:14]

PTAL

[john.yan, 2016-06-07 14:25:20]

The CQ bit was checked by jyan@ca.ibm.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-07 14:25:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2045943002/1

[commit-bot: I haz the power, 2016-06-07 14:54:58]

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

[commit-bot: I haz the power, 2016-06-07 14:54:59]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-06-07 16:33:27]

On 2016/06/07 14:54:59, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Looks like there is a bit of a race situation going on.

https://codereview.chromium.org/2034093002/

Implements big-endian support in a slightly different way that is
platform-independent. It boils down memory accesses into byte-by-byte loads and
then does some work in the instruction selector. That has the advantage that it
also handles misaligned accesses, which is an issue on MIPS.

I think we should use that approach and make the operator that you introduce
here into an optional operator. We can use the optional byte-reverse operator to
generate better code on the platforms where it is supported.

[john.yan, 2016-06-07 17:40:24]

On 2016/06/07 16:33:27, titzer wrote:
> On 2016/06/07 14:54:59, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Looks like there is a bit of a race situation going on.
> 
> https://codereview.chromium.org/2034093002/
> 
> Implements big-endian support in a slightly different way that is
> platform-independent. It boils down memory accesses into byte-by-byte loads
and
> then does some work in the instruction selector. That has the advantage that
it
> also handles misaligned accesses, which is an issue on MIPS.
> 
> I think we should use that approach and make the operator that you introduce
> here into an optional operator. We can use the optional byte-reverse operator
to
> generate better code on the platforms where it is supported.

Thank you for the info.
Yeah, I agree that would really make sense to have both implementation.

[Benedikt Meurer, 2016-06-07 17:51:36]

SGTM

[ivica.bogosavljevic, 2016-06-08 10:27:15]

Hi John!

I am the autor of https://codereview.chromium.org/2034093002/.

Please reuse the parts from BuildChangeEndianness (and rename it to
BuildByteSwap or similar) to implement byte swapping when optional operators are
not available.

Also, you need to add support for your big-endian in wasm-interpreter and fix
the tests too. You can also reuse the existing code.

Ivica

[ivica.bogosavljevic, 2016-06-08 11:17:42]

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

[john.yan, 2016-06-08 13:22:54]

On 2016/06/08 10:27:15, ivica.bogosavljevic wrote:
> Hi John!
> 
> I am the autor of https://codereview.chromium.org/2034093002/.
> 
> Please reuse the parts from BuildChangeEndianness (and rename it to
> BuildByteSwap or similar) to implement byte swapping when optional operators
are
> not available.
> 
> Also, you need to add support for your big-endian in wasm-interpreter and fix
> the tests too. You can also reuse the existing code.
> 
> Ivica

Hello Ivica,

Thanks for the comment. I agree with you. I will probably rebase this on top of
your change. :)

[ivica.bogosavljevic, 2016-06-29 14:21:59]

https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2787: load);
32 bit architectures don't have Word64ReverseByte instructions. I think you will
need to implement Word64ReverseBytes to Word32ReverseBytes in int64-lowering in
order to have everything working on 32 bit architectures

[ivica.bogosavljevic, 2016-07-06 11:28:48]

https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2768: switch (numberOfBytes) {
You are missing here proper handling of floating-point values. Word32ReverseByte
and Word64ReverseByte don't accept float and double, you need to fix this

[john.yan, 2016-07-06 14:35:17]

On 2016/07/06 11:28:48, ivica.bogosavljevic wrote:
>
https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
> src/compiler/wasm-compiler.cc:2768: switch (numberOfBytes) {
> You are missing here proper handling of floating-point values.
Word32ReverseByte
> and Word64ReverseByte don't accept float and double, you need to fix this

Hello ivica, Thanks for the comment. I was busy fixing 5.3 native failures. I
will try to address this soon.

[john.yan, 2016-07-19 18:59:16]

On 2016/07/06 11:28:48, ivica.bogosavljevic wrote:
>
https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2045943002/diff/40001/src/compiler/wasm-compi...
> src/compiler/wasm-compiler.cc:2768: switch (numberOfBytes) {
> You are missing here proper handling of floating-point values.
Word32ReverseByte
> and Word64ReverseByte don't accept float and double, you need to fix this

Hello, latest fix has included int64-lowering implementation and use your
original implementation for float and double.

also cc @titzer, Would you please take an other look at this?

Thanks.

[john.yan, 2016-07-19 19:11:36]

The CQ bit was checked by jyan@ca.ibm.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 19:11:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 19:46:46]

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

[commit-bot: I haz the power, 2016-07-19 19:46:47]

Dry run: This issue passed the CQ dry run.

[john.yan, 2016-07-26 19:27:10]

Description was changed from

==========
[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=
==========

to

==========
[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=
==========

[john.yan, 2016-07-26 19:27:10]

jyan@ca.ibm.com changed reviewers:
+ ahaas@chromium.org

[john.yan, 2016-07-26 19:28:06]

@ahaas@chromium.org Would you please take a look at this CL? Thanks a lot!

[ahaas, 2016-07-27 01:31:15]

I think you should add tests for this CL.

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

https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:630: bool Is16() const { return word() ==
MachineRepresentation::kWord16; }
The introduction of Is16() worries me. Is a word size of 16 actually supported
in V8? At several places in the code there is the assumption that "Is32() ==
(!Is64())". It seems that the introduction of Is16() violates this assumption.

https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
MachineType& machineType) {
Why is the machineType passed in if it is not used?

[john.yan, 2016-07-27 18:14:14]

Patchset #8 (id:140001) has been deleted

[john.yan, 2016-07-27 18:18:19]

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

https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:630: bool Is16() const { return word() ==
MachineRepresentation::kWord16; }
On 2016/07/27 01:31:15, ahaas wrote:
> The introduction of Is16() worries me. Is a word size of 16 actually supported
> in V8? At several places in the code there is the assumption that "Is32() ==
> (!Is64())". It seems that the introduction of Is16() violates this assumption.

Ok, I agree. Word16ReverseBytes Opcode is now removed.

https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
MachineType& machineType) {
On 2016/07/27 01:31:15, ahaas wrote:
> Why is the machineType passed in if it is not used?

Thanks for pointing out. This is my mistake.

[john.yan, 2016-07-27 18:23:26]

On 2016/07/27 01:31:15, ahaas wrote:
> I think you should add tests for this CL.
> 
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> File src/compiler/machine-operator.h (right):
> 
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> src/compiler/machine-operator.h:630: bool Is16() const { return word() ==
> MachineRepresentation::kWord16; }
> The introduction of Is16() worries me. Is a word size of 16 actually supported
> in V8? At several places in the code there is the assumption that "Is32() ==
> (!Is64())". It seems that the introduction of Is16() violates this assumption.
> 
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
> MachineType& machineType) {
> Why is the machineType passed in if it is not used?

These two opcode are used in most of the wasm tests on big-endian platform
(whenever there is a load/store, we need to reverse bytes). On little-endian,
the opcode is not implemented anyway. So I think test coverage are enough to
cover these. Do you think we should add additional dedicate tests?

[ahaas, 2016-07-27 19:35:53]

On 2016/07/27 at 18:23:26, jyan wrote:
> On 2016/07/27 01:31:15, ahaas wrote:
> > I think you should add tests for this CL.
> > 
> >
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > File src/compiler/machine-operator.h (right):
> > 
> >
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > src/compiler/machine-operator.h:630: bool Is16() const { return word() ==
> > MachineRepresentation::kWord16; }
> > The introduction of Is16() worries me. Is a word size of 16 actually
supported
> > in V8? At several places in the code there is the assumption that "Is32() ==
> > (!Is64())". It seems that the introduction of Is16() violates this
assumption.
> > 
> >
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
> > MachineType& machineType) {
> > Why is the machineType passed in if it is not used?
> 
> These two opcode are used in most of the wasm tests on big-endian platform
(whenever there is a load/store, we need to reverse bytes). On little-endian,
the opcode is not implemented anyway. So I think test coverage are enough to
cover these. Do you think we should add additional dedicate tests?

I think it would be good to have basic tests in int64-lowering-unittest.cc,
test-run-wasm.cc, and maybe also test-run-wasm64.cc. The tests are also good for
documentation.

[john.yan, 2016-07-28 04:15:13]

On 2016/07/27 19:35:53, ahaas wrote:
> On 2016/07/27 at 18:23:26, jyan wrote:
> > On 2016/07/27 01:31:15, ahaas wrote:
> > > I think you should add tests for this CL.
> > > 
> > >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > File src/compiler/machine-operator.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > src/compiler/machine-operator.h:630: bool Is16() const { return word() ==
> > > MachineRepresentation::kWord16; }
> > > The introduction of Is16() worries me. Is a word size of 16 actually
> supported
> > > in V8? At several places in the code there is the assumption that "Is32()
==
> > > (!Is64())". It seems that the introduction of Is16() violates this
> assumption.
> > > 
> > >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
> > > MachineType& machineType) {
> > > Why is the machineType passed in if it is not used?
> > 
> > These two opcode are used in most of the wasm tests on big-endian platform
> (whenever there is a load/store, we need to reverse bytes). On little-endian,
> the opcode is not implemented anyway. So I think test coverage are enough to
> cover these. Do you think we should add additional dedicate tests?
> 
> I think it would be good to have basic tests in int64-lowering-unittest.cc,
> test-run-wasm.cc, and maybe also test-run-wasm64.cc. The tests are also good
for
> documentation.

Hello! int64-lowering unittest is implemented in the latest patch. Could you
please enlighten me what should be tested in test-run-wasm(64).cc?
Under current implementation, wasm tests like LoadMemXXX, StoreMemXXX and etc
are already using Word32/64ReverseBytes (if available) undercover in big-endian.

[ivica.bogosavljevic, 2016-07-28 12:19:10]

On 2016/07/28 04:15:13, john.yan wrote:
> On 2016/07/27 19:35:53, ahaas wrote:
> > On 2016/07/27 at 18:23:26, jyan wrote:
> > > On 2016/07/27 01:31:15, ahaas wrote:
> > > > I think you should add tests for this CL.
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > > File src/compiler/machine-operator.h (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > > src/compiler/machine-operator.h:630: bool Is16() const { return word()
==
> > > > MachineRepresentation::kWord16; }
> > > > The introduction of Is16() worries me. Is a word size of 16 actually
> > supported
> > > > in V8? At several places in the code there is the assumption that
"Is32()
> ==
> > > > (!Is64())". It seems that the introduction of Is16() violates this
> > assumption.
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > > src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
> > > > MachineType& machineType) {
> > > > Why is the machineType passed in if it is not used?
> > > 
> > > These two opcode are used in most of the wasm tests on big-endian platform
> > (whenever there is a load/store, we need to reverse bytes). On
little-endian,
> > the opcode is not implemented anyway. So I think test coverage are enough to
> > cover these. Do you think we should add additional dedicate tests?
> > 
> > I think it would be good to have basic tests in int64-lowering-unittest.cc,
> > test-run-wasm.cc, and maybe also test-run-wasm64.cc. The tests are also good
> for
> > documentation.
> 
> Hello! int64-lowering unittest is implemented in the latest patch. Could you
> please enlighten me what should be tested in test-run-wasm(64).cc?
> Under current implementation, wasm tests like LoadMemXXX, StoreMemXXX and etc
> are already using Word32/64ReverseBytes (if available) undercover in
big-endian.

You introduced a new turbofan operator, so you need to write tests to test it.
I guess this is done in test-run-machops.c

[john.yan, 2016-07-28 13:37:45]

On 2016/07/28 12:19:10, ivica.bogosavljevic wrote:
> On 2016/07/28 04:15:13, john.yan wrote:
> > On 2016/07/27 19:35:53, ahaas wrote:
> > > On 2016/07/27 at 18:23:26, jyan wrote:
> > > > On 2016/07/27 01:31:15, ahaas wrote:
> > > > > I think you should add tests for this CL.
> > > > > 
> > > > >
> > >
> >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > > > File src/compiler/machine-operator.h (right):
> > > > > 
> > > > >
> > >
> >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > > > src/compiler/machine-operator.h:630: bool Is16() const { return word()
> ==
> > > > > MachineRepresentation::kWord16; }
> > > > > The introduction of Is16() worries me. Is a word size of 16 actually
> > > supported
> > > > > in V8? At several places in the code there is the assumption that
> "Is32()
> > ==
> > > > > (!Is64())". It seems that the introduction of Is16() violates this
> > > assumption.
> > > > > 
> > > > >
> > >
> >
>
https://codereview.chromium.org/2045943002/diff/120001/src/compiler/machine-o...
> > > > > src/compiler/machine-operator.h:647: bool ReverseBytesSupported(const
> > > > > MachineType& machineType) {
> > > > > Why is the machineType passed in if it is not used?
> > > > 
> > > > These two opcode are used in most of the wasm tests on big-endian
platform
> > > (whenever there is a load/store, we need to reverse bytes). On
> little-endian,
> > > the opcode is not implemented anyway. So I think test coverage are enough
to
> > > cover these. Do you think we should add additional dedicate tests?
> > > 
> > > I think it would be good to have basic tests in
int64-lowering-unittest.cc,
> > > test-run-wasm.cc, and maybe also test-run-wasm64.cc. The tests are also
good
> > for
> > > documentation.
> > 
> > Hello! int64-lowering unittest is implemented in the latest patch. Could you
> > please enlighten me what should be tested in test-run-wasm(64).cc?
> > Under current implementation, wasm tests like LoadMemXXX, StoreMemXXX and
etc
> > are already using Word32/64ReverseBytes (if available) undercover in
> big-endian.
> 
> You introduced a new turbofan operator, so you need to write tests to test it.
> I guess this is done in test-run-machops.c

Thanks! Tests for W32/64ReverseBytes are now implemented.

@ahaas: Would you please take a look? Thanks!

[ahaas, 2016-07-28 14:12:59]

Sorry that I mixed up test-run-wasm.cc and test-run-machops.cc.

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/int64-low...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/int64-low...
src/compiler/int64-lowering.cc:753:
DCHECK(machine()->Word64ReverseBytes().IsSupported());
If the machine already supports Word64ReverseBytes, why do you want to lower? If
the machine does not actually support Word64ReverseBytes, then
machine()->Word64ReverseBytes().IsSupported() should be false.

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

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/machine-o...
src/compiler/machine-operator.h:644: bool ReverseBytesSupported(size_t
size_in_bytes) {
Could this function not just be a helper function in wasm-compiler.cc?

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/machine-o...
src/compiler/machine-operator.h:651: break;
Do you expect that the default case is ever reached? If not, then I would
suggest you put an UNREACHABLE() there.

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

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1063:
graph()->NewNode(m->Word32ReverseBytes().op(),
Why do you sometimes use jsgraph()->machine()->..., and sometimes m->...? I
think you should decide for one and stick with it.

[john.yan, 2016-07-28 18:03:50]

All places are now fixed. PTAL. Thanks!

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/int64-low...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/int64-low...
src/compiler/int64-lowering.cc:753:
DCHECK(machine()->Word64ReverseBytes().IsSupported());
On 2016/07/28 14:12:59, ahaas wrote:
> If the machine already supports Word64ReverseBytes, why do you want to lower?
If
> the machine does not actually support Word64ReverseBytes, then
> machine()->Word64ReverseBytes().IsSupported() should be false.

Check is now removed.

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

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/machine-o...
src/compiler/machine-operator.h:644: bool ReverseBytesSupported(size_t
size_in_bytes) {
On 2016/07/28 14:12:59, ahaas wrote:
> Could this function not just be a helper function in wasm-compiler.cc?

Sure.

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/machine-o...
src/compiler/machine-operator.h:651: break;
On 2016/07/28 14:12:59, ahaas wrote:
> Do you expect that the default case is ever reached? If not, then I would
> suggest you put an UNREACHABLE() there.

No, I expect it to return false (unsupported).

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

https://codereview.chromium.org/2045943002/diff/200001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1063:
graph()->NewNode(m->Word32ReverseBytes().op(),
On 2016/07/28 14:12:59, ahaas wrote:
> Why do you sometimes use jsgraph()->machine()->..., and sometimes m->...? I
> think you should decide for one and stick with it.

Acknowledged.

[ahaas, 2016-07-29 04:15:18]

lgtm with nits, I did not look at the S390 code though.

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

https://codereview.chromium.org/2045943002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1035: switch (memtype.representation()) {
it seems to me that pre-calculating shiftLeftOpcode, shiftRightOpcode,
andOpcode, and orOpcode is unnecessary, because in the code below it is always
obvious which one should be used. For me the use of shiftLeftOpcode was
distracting, and I would prefer to read m->Word64Shl() instead of
shiftLeftOpcode because I am used to that.

nit: I think in V8 local variables do not use camel case but lower case words
with underscores.

https://codereview.chromium.org/2045943002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1071: case 1:
nit: you could remove this case, it should be unreachable because you return
already above when valueSizeInBytes == 1.

https://codereview.chromium.org/2045943002/diff/220001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2045943002/diff/220001/test/cctest/compiler/t...
test/cctest/compiler/test-run-machops.cc:224:
RawMachineAssemblerTester<uint64_t> m(MachineType::Uint64());
Did you intentionally use the RawMachineAssemblerTester instead of the
BufferedRawMachineAssemblerTester here? I think you should use the same for both
tests, and I would prefer the BufferedRawMachineAssemblerTester if you do not
have special reasons for using the RawMachineAssemblerTester because the
BufferedRawMachineAssemblerTester is more robust.

[john.yan, 2016-07-29 17:12:03]

Fixed. Thank you!

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

https://codereview.chromium.org/2045943002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1035: switch (memtype.representation()) {
On 2016/07/29 04:15:18, ahaas wrote:
> it seems to me that pre-calculating shiftLeftOpcode, shiftRightOpcode,
> andOpcode, and orOpcode is unnecessary, because in the code below it is always
> obvious which one should be used. For me the use of shiftLeftOpcode was
> distracting, and I would prefer to read m->Word64Shl() instead of
> shiftLeftOpcode because I am used to that.
> 
> nit: I think in V8 local variables do not use camel case but lower case words
> with underscores. 

xxxOpcode is now removed.

https://codereview.chromium.org/2045943002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1071: case 1:
On 2016/07/29 04:15:18, ahaas wrote:
> nit: you could remove this case, it should be unreachable because you return
> already above when valueSizeInBytes == 1.

Done.

https://codereview.chromium.org/2045943002/diff/220001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2045943002/diff/220001/test/cctest/compiler/t...
test/cctest/compiler/test-run-machops.cc:224:
RawMachineAssemblerTester<uint64_t> m(MachineType::Uint64());
On 2016/07/29 04:15:18, ahaas wrote:
> Did you intentionally use the RawMachineAssemblerTester instead of the
> BufferedRawMachineAssemblerTester here? I think you should use the same for
both
> tests, and I would prefer the BufferedRawMachineAssemblerTester if you do not
> have special reasons for using the RawMachineAssemblerTester because the
> BufferedRawMachineAssemblerTester is more robust.

No. The test case was modified based on RunWord64ReverseBits above. I don't know
why RunWord64ReverseBits uses unbuffered version but RunWord32ReverseBits uses
buffered version.

[john.yan, 2016-07-29 17:17:48]

The CQ bit was checked by jyan@ca.ibm.com

[john.yan, 2016-07-29 17:17:49]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2045943002/#ps260001
(title: "fix typo")

[commit-bot: I haz the power, 2016-07-29 17:18:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 17:21:08]

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

[commit-bot: I haz the power, 2016-07-29 17:21:09]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20581)

[titzer, 2016-07-29 18:21:54]

lgtm with nit

https://codereview.chromium.org/2045943002/diff/260001/src/compiler/int64-low...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/2045943002/diff/260001/src/compiler/int64-low...
src/compiler/int64-lowering.cc:782:
DCHECK(machine()->Word32ReverseBytes().IsSupported());
Can you remove this DCHECK? E.g. the optional operator's .op() method will fail
if it is not supported below.

[john.yan, 2016-07-29 19:08:14]

The CQ bit was unchecked by jyan@ca.ibm.com

[john.yan, 2016-07-29 19:08:36]

On 2016/07/29 18:21:54, titzer wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/2045943002/diff/260001/src/compiler/int64-low...
> File src/compiler/int64-lowering.cc (right):
> 
>
https://codereview.chromium.org/2045943002/diff/260001/src/compiler/int64-low...
> src/compiler/int64-lowering.cc:782:
> DCHECK(machine()->Word32ReverseBytes().IsSupported());
> Can you remove this DCHECK? E.g. the optional operator's .op() method will
fail
> if it is not supported below.

Fixed. Thank you!

[john.yan, 2016-07-29 19:09:38]

The CQ bit was checked by jyan@ca.ibm.com

[john.yan, 2016-07-29 19:09:38]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2045943002/#ps280001
(title: "Fix according to titzer")

[commit-bot: I haz the power, 2016-07-29 19:09:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 19:32:03]

Description was changed from

==========
[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=
==========

to

==========
[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2016-07-29 19:32:04]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-07-29 19:33:38]

Description was changed from

==========
[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=
==========

to

==========
[compiler] [wasm] Introduce Word32/64ReverseBytes as TF Optional Opcode

This commit fixes wasm little-endian load issue on big-endian platform
by introducing reverse byte operation immediately after a load.

R=bmeurer@chromium.org, titzer@chromium.org
BUG=

Committed: https://crrev.com/77c9cb8341e85e8e2f1b65d5004ac90e7dc060ea
Cr-Commit-Position: refs/heads/master@{#38183}
==========

[commit-bot: I haz the power, 2016-07-29 19:33:40]

Patchset 14 (id:??) landed as
https://crrev.com/77c9cb8341e85e8e2f1b65d5004ac90e7dc060ea
Cr-Commit-Position: refs/heads/master@{#38183}