[wasm] Implement 128-bit endian swap for simd type

[wasm] Implement 128-bit endian swap for simd type

BUG=

Review-Url: https://codereview.chromium.org/2838943002
Cr-Commit-Position: refs/heads/master@{#45208}
Committed: https://chromium.googlesource.com/v8/v8/+/18c33c504a9ba61df2fa3833210f83539d2569f4

[john.yan, 2017-04-25 18:55:06]

Description was changed from

==========
[wasm] Implement 128-bit endian swap for simd type

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

to

==========
[wasm] Implement 128-bit endian swap for simd type

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

[john.yan, 2017-04-25 18:55:07]

jyan@ca.ibm.com changed reviewers:
+ bbudge@chromium.org, bjaideep@ca.ibm.com, clemensh@chromium.org,
joransiu@ca.ibm.com

[john.yan, 2017-04-25 18:55:15]

ptal

[john.yan, 2017-04-25 18:57:25]

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

[commit-bot: I haz the power, 2017-04-25 18:57:35]

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, 2017-04-25 19:23:57]

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

[commit-bot: I haz the power, 2017-04-25 19:23:58]

Dry run: This issue passed the CQ dry run.

[john.yan, 2017-04-26 16:58:00]

@clemensh@google.com Could you please take a look?

[Clemens Hammacher, 2017-04-26 18:40:06]

Sorry for the delay!

https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:1072: case 16:
You can merge this with the case for 4.

https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:1108: DCHECK(ReverseBytesSupported(m, 4));
You meant 16, not 4. You can also use valueSizeInBytes here.

https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-simd.cc:2215: r.module().WriteMemory(&memory[3],
expected);
This does not look like the right fix. Storing the lanes in different order
would make endianess observable, breaking platform independence of wasm code.
I am no expert in the SIMD proposal, though.

Bill, can you comment on that?

[john.yan, 2017-04-26 18:56:43]

On 2017/04/26 18:40:06, Clemens Hammacher wrote:
> Sorry for the delay!
> 
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:1072: case 16:
> You can merge this with the case for 4.
> 
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:1108: DCHECK(ReverseBytesSupported(m, 4));
> You meant 16, not 4. You can also use valueSizeInBytes here.
> 
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> File test/cctest/wasm/test-run-wasm-simd.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> test/cctest/wasm/test-run-wasm-simd.cc:2215:
r.module().WriteMemory(&memory[3],
> expected);
> This does not look like the right fix. Storing the lanes in different order
> would make endianess observable, breaking platform independence of wasm code.
> I am no expert in the SIMD proposal, though.
> 
> Bill, can you comment on that?

Thanks for your comments! and updated.

[bbudge, 2017-04-26 20:14:12]

https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:1135: for (int lane = 0; lane < 4; lane++)
use braces for this loop body

and below

https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-simd.cc:2215: r.module().WriteMemory(&memory[3],
expected);
On 2017/04/26 18:40:05, Clemens Hammacher wrote:
> This does not look like the right fix. Storing the lanes in different order
> would make endianess observable, breaking platform independence of wasm code.
> I am no expert in the SIMD proposal, though.
> 
> Bill, can you comment on that?

You're right Clemens. SIMD shouldn't be any different than the rest of
WebAssembly. Loads and stores are always in little-endian order, so this test
shouldn't change.

[john.yan, 2017-04-27 17:09:37]

On 2017/04/26 20:14:12, bbudge wrote:
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:1135: for (int lane = 0; lane < 4; lane++)
> use braces for this loop body
> 
> and below
> 
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> File test/cctest/wasm/test-run-wasm-simd.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> test/cctest/wasm/test-run-wasm-simd.cc:2215:
r.module().WriteMemory(&memory[3],
> expected);
> On 2017/04/26 18:40:05, Clemens Hammacher wrote:
> > This does not look like the right fix. Storing the lanes in different order
> > would make endianess observable, breaking platform independence of wasm
code.
> > I am no expert in the SIMD proposal, though.
> > 
> > Bill, can you comment on that?
> 
> You're right Clemens. SIMD shouldn't be any different than the rest of
> WebAssembly. Loads and stores are always in little-endian order, so this test
> shouldn't change.

Hello Bill,

thank you for your comment.
Just one question though: 
Is wasm Global memory in LE order as well? eg, need to change endian order for
Get/SetGlobal.

Regards,
John

[bbudge, 2017-04-27 17:14:02]

On 2017/04/27 17:09:37, john.yan wrote:
> On 2017/04/26 20:14:12, bbudge wrote:
> >
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
> > File src/compiler/wasm-compiler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
> > src/compiler/wasm-compiler.cc:1135: for (int lane = 0; lane < 4; lane++)
> > use braces for this loop body
> > 
> > and below
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> > File test/cctest/wasm/test-run-wasm-simd.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> > test/cctest/wasm/test-run-wasm-simd.cc:2215:
> r.module().WriteMemory(&memory[3],
> > expected);
> > On 2017/04/26 18:40:05, Clemens Hammacher wrote:
> > > This does not look like the right fix. Storing the lanes in different
order
> > > would make endianess observable, breaking platform independence of wasm
> code.
> > > I am no expert in the SIMD proposal, though.
> > > 
> > > Bill, can you comment on that?
> > 
> > You're right Clemens. SIMD shouldn't be any different than the rest of
> > WebAssembly. Loads and stores are always in little-endian order, so this
test
> > shouldn't change.
> 
> Hello Bill,
> 
> thank you for your comment.
> Just one question though: 
> Is wasm Global memory in LE order as well? eg, need to change endian order for
> Get/SetGlobal.
> 
> Regards,
> John

Hi John,

Yes, globals too. Otherwise, WASM code wouldn't really be portable.

Bill

[bbudge, 2017-04-27 17:14:03]

On 2017/04/27 17:09:37, john.yan wrote:
> On 2017/04/26 20:14:12, bbudge wrote:
> >
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
> > File src/compiler/wasm-compiler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
> > src/compiler/wasm-compiler.cc:1135: for (int lane = 0; lane < 4; lane++)
> > use braces for this loop body
> > 
> > and below
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> > File test/cctest/wasm/test-run-wasm-simd.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> > test/cctest/wasm/test-run-wasm-simd.cc:2215:
> r.module().WriteMemory(&memory[3],
> > expected);
> > On 2017/04/26 18:40:05, Clemens Hammacher wrote:
> > > This does not look like the right fix. Storing the lanes in different
order
> > > would make endianess observable, breaking platform independence of wasm
> code.
> > > I am no expert in the SIMD proposal, though.
> > > 
> > > Bill, can you comment on that?
> > 
> > You're right Clemens. SIMD shouldn't be any different than the rest of
> > WebAssembly. Loads and stores are always in little-endian order, so this
test
> > shouldn't change.
> 
> Hello Bill,
> 
> thank you for your comment.
> Just one question though: 
> Is wasm Global memory in LE order as well? eg, need to change endian order for
> Get/SetGlobal.
> 
> Regards,
> John

Hi John,

Yes, globals too. Otherwise, WASM code wouldn't really be portable.

Bill

[Clemens Hammacher, 2017-04-27 17:22:45]

On 2017/04/27 at 17:14:02, bbudge wrote:
> On 2017/04/27 17:09:37, john.yan wrote:
> > On 2017/04/26 20:14:12, bbudge wrote:
> > >
> >
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler.cc
> > > File src/compiler/wasm-compiler.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2838943002/diff/1/src/compiler/wasm-compiler....
> > > src/compiler/wasm-compiler.cc:1135: for (int lane = 0; lane < 4; lane++)
> > > use braces for this loop body
> > > 
> > > and below
> > > 
> > >
> >
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> > > File test/cctest/wasm/test-run-wasm-simd.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2838943002/diff/1/test/cctest/wasm/test-run-w...
> > > test/cctest/wasm/test-run-wasm-simd.cc:2215:
> > r.module().WriteMemory(&memory[3],
> > > expected);
> > > On 2017/04/26 18:40:05, Clemens Hammacher wrote:
> > > > This does not look like the right fix. Storing the lanes in different
order
> > > > would make endianess observable, breaking platform independence of wasm
> > code.
> > > > I am no expert in the SIMD proposal, though.
> > > > 
> > > > Bill, can you comment on that?
> > > 
> > > You're right Clemens. SIMD shouldn't be any different than the rest of
> > > WebAssembly. Loads and stores are always in little-endian order, so this
test
> > > shouldn't change.
> > 
> > Hello Bill,
> > 
> > thank you for your comment.
> > Just one question though: 
> > Is wasm Global memory in LE order as well? eg, need to change endian order
for
> > Get/SetGlobal.
> > 
> > Regards,
> > John
> 
> Hi John,
> 
> Yes, globals too. Otherwise, WASM code wouldn't really be portable.
> 
> Bill

Wait, but global memory is not observable, right? So as long as you read them
back the same way you wrote them, that's fine. You only have to be careful about
the initializers.

[john.yan, 2017-04-27 17:44:48]

https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd.cc:2193: CHECK_EQ(*global, 13.5);
I am not sure I am on the right tract. If global memory is BE, then these
CHECK_EQ should be in reversed order. If global memory has to be LE, there are a
lot of places need to update in test-run-wasm.cc

[Clemens Hammacher, 2017-04-27 17:48:33]

https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd.cc:2193: CHECK_EQ(*global, 13.5);
On 2017/04/27 at 17:44:48, john.yan wrote:
> I am not sure I am on the right tract. If global memory is BE, then these
CHECK_EQ should be in reversed order. If global memory has to be LE, there are a
lot of places need to update in test-run-wasm.cc

OK, let me correct myself: Tests are the only way to observe byte order of
stored globals :/
It would be OK for me to fix the tests in this case. But *only* for global
memory, not for the general memory, as byte order in general memory is
observable.

Bill, feel free to object :)

[bbudge, 2017-04-27 18:06:55]

On 2017/04/27 17:48:33, Clemens Hammacher wrote:
>
https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
> File test/cctest/wasm/test-run-wasm-simd.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
> test/cctest/wasm/test-run-wasm-simd.cc:2193: CHECK_EQ(*global, 13.5);
> On 2017/04/27 at 17:44:48, john.yan wrote:
> > I am not sure I am on the right tract. If global memory is BE, then these
> CHECK_EQ should be in reversed order. If global memory has to be LE, there are
a
> lot of places need to update in test-run-wasm.cc
> 
> OK, let me correct myself: Tests are the only way to observe byte order of
> stored globals :/
> It would be OK for me to fix the tests in this case. But *only* for global
> memory, not for the general memory, as byte order in general memory is
> observable.
> 
> Bill, feel free to object :)

No objections. SIMD should behave the same way the rest of WASM does.

[john.yan, 2017-04-27 20:05:44]

jyan@ca.ibm.com changed reviewers:
+ jarin@chromium.org, mstarzinger@chromium.org

[john.yan, 2017-04-27 20:09:59]

On 2017/04/27 17:48:33, Clemens Hammacher wrote:
>
https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
> File test/cctest/wasm/test-run-wasm-simd.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/20001/test/cctest/wasm/test-r...
> test/cctest/wasm/test-run-wasm-simd.cc:2193: CHECK_EQ(*global, 13.5);
> On 2017/04/27 at 17:44:48, john.yan wrote:
> > I am not sure I am on the right tract. If global memory is BE, then these
> CHECK_EQ should be in reversed order. If global memory has to be LE, there are
a
> lot of places need to update in test-run-wasm.cc
> 
> OK, let me correct myself: Tests are the only way to observe byte order of
> stored globals :/
> It would be OK for me to fix the tests in this case. But *only* for global
> memory, not for the general memory, as byte order in general memory is
> observable.
> 
> Bill, feel free to object :)

Hello, I have updated the Get/SetGlobal tests to respect native endian order.
And also fix 2 places related to endian orders in simd scalar lowering.
Could you please take an other look?

Thanks,
John

[john.yan, 2017-04-27 20:36:26]

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

[commit-bot: I haz the power, 2017-04-27 20:36:37]

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, 2017-04-27 20:39:07]

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

[commit-bot: I haz the power, 2017-04-27 20:39:08]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/25025)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/21623)

[john.yan, 2017-04-27 20:51:58]

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

[john.yan, 2017-04-27 20:52:09]

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

[commit-bot: I haz the power, 2017-04-27 20:52:17]

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, 2017-04-27 20:54:47]

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

[commit-bot: I haz the power, 2017-04-27 20:54:48]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/25119)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/25108)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/25026)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/21624)

[john.yan, 2017-04-27 21:01:14]

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

[john.yan, 2017-04-27 21:02:32]

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

[commit-bot: I haz the power, 2017-04-27 21:02:35]

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, 2017-04-27 21:05:05]

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

[commit-bot: I haz the power, 2017-04-27 21:05:06]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[john.yan, 2017-04-27 21:12:35]

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

[john.yan, 2017-04-27 21:21:09]

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

[commit-bot: I haz the power, 2017-04-27 21:21:15]

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, 2017-04-27 21:27:04]

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

[commit-bot: I haz the power, 2017-04-27 21:27:05]

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

[john.yan, 2017-04-27 22:53:30]

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

[commit-bot: I haz the power, 2017-04-27 22:53:33]

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, 2017-04-27 22:59:43]

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

[commit-bot: I haz the power, 2017-04-27 22:59:44]

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

[john.yan, 2017-04-28 02:59:17]

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

[john.yan, 2017-04-28 02:59:27]

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

[commit-bot: I haz the power, 2017-04-28 02:59:33]

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, 2017-04-28 03:29:49]

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

[commit-bot: I haz the power, 2017-04-28 03:29:50]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-04-28 09:45:48]

That looks much better now! Just a few minor comments.

https://codereview.chromium.org/2838943002/diff/180001/src/compiler/simd-scal...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2838943002/diff/180001/src/compiler/simd-scal...
src/compiler/simd-scalar-lowering.cc:203: #if defined(V8_TARGET_BIG_ENDIAN)
It looks like this code would get a lot cleaner if we introduce something like
this in the header:

#if defined(V8_TARGET_BIG_ENDIAN)                                               
   
  static constexpr int kLaneOffsets[] = {3, 2, 1, 0};                           

#else                                                                           
   
  static constexpr int kLaneOffsets[] = {0, 1, 2, 3};                           
   
#endif

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

https://codereview.chromium.org/2838943002/diff/180001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1142:
graph()->NewNode(jsgraph()->machine()->S128And(), value, value);
Wouldn't "result = value;" suffice? The I32x4ReplaceLane should generate a new
value, no?

https://codereview.chromium.org/2838943002/diff/180001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2838943002/diff/180001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:2005: const int kElems = vSize /
sizeof(T);
You can make this (and the others) constexpr, move {kElems} above the "#if", and
reuse it in the DCHECK.

[john.yan, 2017-04-28 15:01:28]

Thanks for comments! I will update soon.

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

https://codereview.chromium.org/2838943002/diff/180001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1142:
graph()->NewNode(jsgraph()->machine()->S128And(), value, value);
On 2017/04/28 09:45:47, Clemens Hammacher wrote:
> Wouldn't "result = value;" suffice? The I32x4ReplaceLane should generate a new
> value, no?

That's exactly what I did for the first time. I think it's fine if SIMD is
enabled. SIMD lowering for I32x4ReplaceLane would actually replaced the old lane
with the new value, but doesn't generate a new vector. So I was running in a
problem ending up messing with order with I32x4ExtractLane above.

[john.yan, 2017-04-28 19:07:05]

On 2017/04/28 09:45:48, Clemens Hammacher wrote:
> That looks much better now! Just a few minor comments.
> 
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/simd-scal...
> File src/compiler/simd-scalar-lowering.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/simd-scal...
> src/compiler/simd-scalar-lowering.cc:203: #if defined(V8_TARGET_BIG_ENDIAN)
> It looks like this code would get a lot cleaner if we introduce something like
> this in the header:
> 
> #if defined(V8_TARGET_BIG_ENDIAN)                                             
 
>    
>   static constexpr int kLaneOffsets[] = {3, 2, 1, 0};                         
 
> 
> #else                                                                         
 
>    
>   static constexpr int kLaneOffsets[] = {0, 1, 2, 3};                         
 
>    
> #endif
> 
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/wasm-comp...
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/wasm-comp...
> src/compiler/wasm-compiler.cc:1142:
> graph()->NewNode(jsgraph()->machine()->S128And(), value, value);
> Wouldn't "result = value;" suffice? The I32x4ReplaceLane should generate a new
> value, no?
> 
>
https://codereview.chromium.org/2838943002/diff/180001/test/cctest/wasm/test-...
> File test/cctest/wasm/test-run-wasm-simd.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/180001/test/cctest/wasm/test-...
> test/cctest/wasm/test-run-wasm-simd.cc:2005: const int kElems = vSize /
> sizeof(T);
> You can make this (and the others) constexpr, move {kElems} above the "#if",
and
> reuse it in the DCHECK.

CL has been updated! Could you please take another look?

Thanks,
John

[john.yan, 2017-04-28 19:07:12]

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

[commit-bot: I haz the power, 2017-04-28 19:07:17]

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, 2017-04-28 19:11:07]

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

[commit-bot: I haz the power, 2017-04-28 19:11:08]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/26786)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)

[john.yan, 2017-04-28 19:25:56]

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

[john.yan, 2017-04-28 19:28:31]

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

[commit-bot: I haz the power, 2017-04-28 19:28:37]

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, 2017-04-28 19:56:42]

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

[commit-bot: I haz the power, 2017-04-28 19:56:43]

Dry run: This issue passed the CQ dry run.

[john.yan, 2017-05-01 17:44:28]

On 2017/04/28 19:07:05, john.yan wrote:
> On 2017/04/28 09:45:48, Clemens Hammacher wrote:
> > That looks much better now! Just a few minor comments.
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/simd-scal...
> > File src/compiler/simd-scalar-lowering.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/simd-scal...
> > src/compiler/simd-scalar-lowering.cc:203: #if defined(V8_TARGET_BIG_ENDIAN)
> > It looks like this code would get a lot cleaner if we introduce something
like
> > this in the header:
> > 
> > #if defined(V8_TARGET_BIG_ENDIAN)                                           
 
>  
> >    
> >   static constexpr int kLaneOffsets[] = {3, 2, 1, 0};                       
 
>  
> > 
> > #else                                                                       
 
>  
> >    
> >   static constexpr int kLaneOffsets[] = {0, 1, 2, 3};                       
 
>  
> >    
> > #endif
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/wasm-comp...
> > File src/compiler/wasm-compiler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/180001/src/compiler/wasm-comp...
> > src/compiler/wasm-compiler.cc:1142:
> > graph()->NewNode(jsgraph()->machine()->S128And(), value, value);
> > Wouldn't "result = value;" suffice? The I32x4ReplaceLane should generate a
new
> > value, no?
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/180001/test/cctest/wasm/test-...
> > File test/cctest/wasm/test-run-wasm-simd.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/180001/test/cctest/wasm/test-...
> > test/cctest/wasm/test-run-wasm-simd.cc:2005: const int kElems = vSize /
> > sizeof(T);
> > You can make this (and the others) constexpr, move {kElems} above the "#if",
> and
> > reuse it in the DCHECK.
> 
> CL has been updated! Could you please take another look?
> 
> Thanks,
> John

Just a reminder. :)

[bbudge, 2017-05-01 18:21:05]

bbudge@chromium.org changed reviewers:
+ aseemgarg@chromium.org

[bbudge, 2017-05-01 18:21:07]

+Aseem for simd lowering changes.

https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:1968: template <typename T, int maxLanes
= 4>
How about 'numLanes' or 'LANES'? 'maxLanes' implies there might be fewer lanes.

https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:1969: void SetVectorByLanes(T* v, ...) {
Could you follow the pattern set by RunUnaryLaneOp and RunBinaryLaneOp, which
uses std::array instead of varargs?

https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:2002: template <int vSize = 16, typename
T>
Simd vectors are always 16 bytes. Why not just use kSimd128Size below?

https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:2003: T GetScalarByLanes(T* v, int lane)
{
Naming suggestion: GetScalar or ExtractLane.

const T& value (or vec, or vector.)

[john.yan, 2017-05-01 19:15:35]

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

[commit-bot: I haz the power, 2017-05-01 19:15:45]

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, 2017-05-01 19:47:49]

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

[commit-bot: I haz the power, 2017-05-01 19:47:50]

Dry run: This issue passed the CQ dry run.

[john.yan, 2017-05-01 19:58:21]

On 2017/05/01 18:21:07, bbudge wrote:
> +Aseem for simd lowering changes.
> 
>
https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
> File test/cctest/wasm/test-run-wasm-simd.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
> test/cctest/wasm/test-run-wasm-simd.cc:1968: template <typename T, int
maxLanes
> = 4>
> How about 'numLanes' or 'LANES'? 'maxLanes' implies there might be fewer
lanes.
> 
>
https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
> test/cctest/wasm/test-run-wasm-simd.cc:1969: void SetVectorByLanes(T* v, ...)
{
> Could you follow the pattern set by RunUnaryLaneOp and RunBinaryLaneOp, which
> uses std::array instead of varargs?
> 
>
https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
> test/cctest/wasm/test-run-wasm-simd.cc:2002: template <int vSize = 16,
typename
> T>
> Simd vectors are always 16 bytes. Why not just use kSimd128Size below?
> 
>
https://codereview.chromium.org/2838943002/diff/240001/test/cctest/wasm/test-...
> test/cctest/wasm/test-run-wasm-simd.cc:2003: T GetScalarByLanes(T* v, int
lane)
> {
> Naming suggestion: GetScalar or ExtractLane.
> 
> const T& value (or vec, or vector.)

Hello Bill,
Thanks for the comments. The CL is updated. Could you please take another look?

Thanks,
John

[Clemens Hammacher, 2017-05-02 09:19:52]

wasm-compiler.cc LGTM with nit

Also no concerns about the other changes, but please wait for Aseems LGTM before
landing.
Thanks!

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

https://codereview.chromium.org/2838943002/diff/260001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1107: DCHECK(ReverseBytesSupported(m, 16));
I would still prefer using "valueSizeInBytes" here instead of the constant.

[aseemgarg, 2017-05-02 21:20:21]

I am entirely sure if the scalar-lowering part is needed. As I understand, the
BuildChangeEndianness will be called at the time the graph is built. In that
case, at the time of lowering we shouldn't need to once again consider the
indices.

https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
indices[0]);
This is not correct. The else case (without control and effect nodes) is not
covered. Also, this needs to be rebased. The code has changed somewhat over here
(Int16x8 was added).

[aseemgarg, 2017-05-02 21:20:59]

On 2017/05/02 21:20:21, aseemgarg wrote:
> I am entirely sure if the scalar-lowering part is needed. As I understand, the
> BuildChangeEndianness will be called at the time the graph is built. In that
> case, at the time of lowering we shouldn't need to once again consider the
> indices.
> 
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> File src/compiler/simd-scalar-lowering.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
> indices[0]);
> This is not correct. The else case (without control and effect nodes) is not
> covered. Also, this needs to be rebased. The code has changed somewhat over
here
> (Int16x8 was added).

*I am *not* entirely sure....

[john.yan, 2017-05-02 21:40:46]

On 2017/05/02 21:20:59, aseemgarg wrote:
> On 2017/05/02 21:20:21, aseemgarg wrote:
> > I am entirely sure if the scalar-lowering part is needed. As I understand,
the
> > BuildChangeEndianness will be called at the time the graph is built. In that
> > case, at the time of lowering we shouldn't need to once again consider the
> > indices.
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > File src/compiler/simd-scalar-lowering.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
> > indices[0]);
> > This is not correct. The else case (without control and effect nodes) is not
> > covered. Also, this needs to be rebased. The code has changed somewhat over
> here
> > (Int16x8 was added).
> 
> *I am *not* entirely sure....

Hello, thanks for the comments!
The problem is that BuildChangeEndianness only concern about LoadMem and
StoreMem but Get/SetGlobal which assumes native  endianness, doesn't call
BuildChangeEndianness.

[aseemgarg, 2017-05-02 21:55:43]

On 2017/05/02 21:40:46, john.yan wrote:
> On 2017/05/02 21:20:59, aseemgarg wrote:
> > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > I am entirely sure if the scalar-lowering part is needed. As I understand,
> the
> > > BuildChangeEndianness will be called at the time the graph is built. In
that
> > > case, at the time of lowering we shouldn't need to once again consider the
> > > indices.
> > > 
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > File src/compiler/simd-scalar-lowering.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
> > > indices[0]);
> > > This is not correct. The else case (without control and effect nodes) is
not
> > > covered. Also, this needs to be rebased. The code has changed somewhat
over
> > here
> > > (Int16x8 was added).
> > 
> > *I am *not* entirely sure....
> 
> Hello, thanks for the comments!
> The problem is that BuildChangeEndianness only concern about LoadMem and
> StoreMem but Get/SetGlobal which assumes native  endianness, doesn't call
> BuildChangeEndianness.

Oh. In that case we need to deal with the two separately? As it is right now, it
seems  incorrect for non global case.

[john.yan, 2017-05-03 17:53:35]

On 2017/05/02 21:55:43, aseemgarg wrote:
> On 2017/05/02 21:40:46, john.yan wrote:
> > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > I am entirely sure if the scalar-lowering part is needed. As I
understand,
> > the
> > > > BuildChangeEndianness will be called at the time the graph is built. In
> that
> > > > case, at the time of lowering we shouldn't need to once again consider
the
> > > > indices.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
> > > > indices[0]);
> > > > This is not correct. The else case (without control and effect nodes) is
> not
> > > > covered. Also, this needs to be rebased. The code has changed somewhat
> over
> > > here
> > > > (Int16x8 was added).
> > > 
> > > *I am *not* entirely sure....
> > 
> > Hello, thanks for the comments!
> > The problem is that BuildChangeEndianness only concern about LoadMem and
> > StoreMem but Get/SetGlobal which assumes native  endianness, doesn't call
> > BuildChangeEndianness.
> 
> Oh. In that case we need to deal with the two separately? As it is right now,
it
> seems  incorrect for non global case.

Hello, I think simd lowering is not only for wasm but also for JS in general,
which should respect native endianess, right? In wasm case, non global case
(little endian) will go through LoadMem/StoreMem, which does the swap for the
lanes, So lanes numbering would be the same as local order (big endian).

[john.yan, 2017-05-03 19:28:10]

Patchset #16 (id:300001) has been deleted

[john.yan, 2017-05-03 19:39:40]

Rebased on master. Could you please take another look?

Thanks!

https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
indices[0]);
On 2017/05/02 21:20:21, aseemgarg wrote:
> This is not correct. The else case (without control and effect nodes) is not
> covered. Also, this needs to be rebased. The code has changed somewhat over
here
> (Int16x8 was added). 

Hello, do you mean the else case below? I don't understand how it will affect
anything from above. Could you please explain a bit more?

Thanks,
John

[john.yan, 2017-05-03 19:39:57]

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

[commit-bot: I haz the power, 2017-05-03 19:40:04]

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, 2017-05-03 19:42:54]

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

[commit-bot: I haz the power, 2017-05-03 19:42:55]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/25416)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)

[john.yan, 2017-05-03 19:57:53]

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

[commit-bot: I haz the power, 2017-05-03 19:58:04]

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, 2017-05-03 20:49:51]

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

[commit-bot: I haz the power, 2017-05-03 20:49:52]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-05-03 22:03:53]

On 2017/05/03 17:53:35, john.yan wrote:
> On 2017/05/02 21:55:43, aseemgarg wrote:
> > On 2017/05/02 21:40:46, john.yan wrote:
> > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > I am entirely sure if the scalar-lowering part is needed. As I
> understand,
> > > the
> > > > > BuildChangeEndianness will be called at the time the graph is built.
In
> > that
> > > > > case, at the time of lowering we shouldn't need to once again consider
> the
> > > > > indices.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > src/compiler/simd-scalar-lowering.cc:237:
rep_nodes[0]->ReplaceInput(1,
> > > > > indices[0]);
> > > > > This is not correct. The else case (without control and effect nodes)
is
> > not
> > > > > covered. Also, this needs to be rebased. The code has changed somewhat
> > over
> > > > here
> > > > > (Int16x8 was added).
> > > > 
> > > > *I am *not* entirely sure....
> > > 
> > > Hello, thanks for the comments!
> > > The problem is that BuildChangeEndianness only concern about LoadMem and
> > > StoreMem but Get/SetGlobal which assumes native  endianness, doesn't call
> > > BuildChangeEndianness.
> > 
> > Oh. In that case we need to deal with the two separately? As it is right
now,
> it
> > seems  incorrect for non global case.
> 
> Hello, I think simd lowering is not only for wasm but also for JS in general,
> which should respect native endianess, right? In wasm case, non global case
> (little endian) will go through LoadMem/StoreMem, which does the swap for the
> lanes, So lanes numbering would be the same as local order (big endian).

Currently, the proposal to add SIMD to JS has been dropped. So, we don't need to
worry about non WASM case.

[aseemgarg, 2017-05-03 22:04:14]

https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
indices[0]);
On 2017/05/03 19:39:40, john.yan wrote:
> On 2017/05/02 21:20:21, aseemgarg wrote:
> > This is not correct. The else case (without control and effect nodes) is not
> > covered. Also, this needs to be rebased. The code has changed somewhat over
> here
> > (Int16x8 was added). 
> 
> Hello, do you mean the else case below? I don't understand how it will affect
> anything from above. Could you please explain a bit more?
> 
> Thanks,
> John

yes the else case below. Since the indices have potentially moved around, the
index for rep_node[0] should be fixed even for the else case.

[john.yan, 2017-05-04 14:38:07]

On 2017/05/03 22:04:14, aseemgarg wrote:
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> File src/compiler/simd-scalar-lowering.cc (right):
> 
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> src/compiler/simd-scalar-lowering.cc:237: rep_nodes[0]->ReplaceInput(1,
> indices[0]);
> On 2017/05/03 19:39:40, john.yan wrote:
> > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > This is not correct. The else case (without control and effect nodes) is
not
> > > covered. Also, this needs to be rebased. The code has changed somewhat
over
> > here
> > > (Int16x8 was added). 
> > 
> > Hello, do you mean the else case below? I don't understand how it will
affect
> > anything from above. Could you please explain a bit more?
> > 
> > Thanks,
> > John
> 
> yes the else case below. Since the indices have potentially moved around, the
> index for rep_node[0] should be fixed even for the else case.

Got it. Thanks!

[john.yan, 2017-05-04 18:50:55]

On 2017/05/03 22:03:53, aseemgarg wrote:
> On 2017/05/03 17:53:35, john.yan wrote:
> > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > I am entirely sure if the scalar-lowering part is needed. As I
> > understand,
> > > > the
> > > > > > BuildChangeEndianness will be called at the time the graph is built.
> In
> > > that
> > > > > > case, at the time of lowering we shouldn't need to once again
consider
> > the
> > > > > > indices.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > src/compiler/simd-scalar-lowering.cc:237:
> rep_nodes[0]->ReplaceInput(1,
> > > > > > indices[0]);
> > > > > > This is not correct. The else case (without control and effect
nodes)
> is
> > > not
> > > > > > covered. Also, this needs to be rebased. The code has changed
somewhat
> > > over
> > > > > here
> > > > > > (Int16x8 was added).
> > > > > 
> > > > > *I am *not* entirely sure....
> > > > 
> > > > Hello, thanks for the comments!
> > > > The problem is that BuildChangeEndianness only concern about LoadMem and
> > > > StoreMem but Get/SetGlobal which assumes native  endianness, doesn't
call
> > > > BuildChangeEndianness.
> > > 
> > > Oh. In that case we need to deal with the two separately? As it is right
> now,
> > it
> > > seems  incorrect for non global case.
> > 
> > Hello, I think simd lowering is not only for wasm but also for JS in
general,
> > which should respect native endianess, right? In wasm case, non global case
> > (little endian) will go through LoadMem/StoreMem, which does the swap for
the
> > lanes, So lanes numbering would be the same as local order (big endian).
> 
> Currently, the proposal to add SIMD to JS has been dropped. So, we don't need
to
> worry about non WASM case.

Hello, thanks for the comments.
For wasm, we still need to differentiate global and non global case. So I think
for now, wasm-compiler will change LE to native endian and leave simd lowering
to respect native endian. Is that ok?

Thanks,
John

[john.yan, 2017-05-04 18:51:08]

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

[commit-bot: I haz the power, 2017-05-04 18:51:14]

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, 2017-05-04 19:23:23]

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

[commit-bot: I haz the power, 2017-05-04 19:23:25]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-05-04 21:35:23]

On 2017/05/04 18:50:55, john.yan wrote:
> On 2017/05/03 22:03:53, aseemgarg wrote:
> > On 2017/05/03 17:53:35, john.yan wrote:
> > > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > > I am entirely sure if the scalar-lowering part is needed. As I
> > > understand,
> > > > > the
> > > > > > > BuildChangeEndianness will be called at the time the graph is
built.
> > In
> > > > that
> > > > > > > case, at the time of lowering we shouldn't need to once again
> consider
> > > the
> > > > > > > indices.
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > src/compiler/simd-scalar-lowering.cc:237:
> > rep_nodes[0]->ReplaceInput(1,
> > > > > > > indices[0]);
> > > > > > > This is not correct. The else case (without control and effect
> nodes)
> > is
> > > > not
> > > > > > > covered. Also, this needs to be rebased. The code has changed
> somewhat
> > > > over
> > > > > > here
> > > > > > > (Int16x8 was added).
> > > > > > 
> > > > > > *I am *not* entirely sure....
> > > > > 
> > > > > Hello, thanks for the comments!
> > > > > The problem is that BuildChangeEndianness only concern about LoadMem
and
> > > > > StoreMem but Get/SetGlobal which assumes native  endianness, doesn't
> call
> > > > > BuildChangeEndianness.
> > > > 
> > > > Oh. In that case we need to deal with the two separately? As it is right
> > now,
> > > it
> > > > seems  incorrect for non global case.
> > > 
> > > Hello, I think simd lowering is not only for wasm but also for JS in
> general,
> > > which should respect native endianess, right? In wasm case, non global
case
> > > (little endian) will go through LoadMem/StoreMem, which does the swap for
> the
> > > lanes, So lanes numbering would be the same as local order (big endian).
> > 
> > Currently, the proposal to add SIMD to JS has been dropped. So, we don't
need
> to
> > worry about non WASM case.
> 
> Hello, thanks for the comments.
> For wasm, we still need to differentiate global and non global case. So I
think
> for now, wasm-compiler will change LE to native endian and leave simd lowering
> to respect native endian. Is that ok?
> 
> Thanks,
> John

Yeah. Seems like that should work. Do test it before submitting.

[john.yan, 2017-05-05 17:29:51]

On 2017/05/04 21:35:23, aseemgarg wrote:
> On 2017/05/04 18:50:55, john.yan wrote:
> > On 2017/05/03 22:03:53, aseemgarg wrote:
> > > On 2017/05/03 17:53:35, john.yan wrote:
> > > > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > > > I am entirely sure if the scalar-lowering part is needed. As I
> > > > understand,
> > > > > > the
> > > > > > > > BuildChangeEndianness will be called at the time the graph is
> built.
> > > In
> > > > > that
> > > > > > > > case, at the time of lowering we shouldn't need to once again
> > consider
> > > > the
> > > > > > > > indices.
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > src/compiler/simd-scalar-lowering.cc:237:
> > > rep_nodes[0]->ReplaceInput(1,
> > > > > > > > indices[0]);
> > > > > > > > This is not correct. The else case (without control and effect
> > nodes)
> > > is
> > > > > not
> > > > > > > > covered. Also, this needs to be rebased. The code has changed
> > somewhat
> > > > > over
> > > > > > > here
> > > > > > > > (Int16x8 was added).
> > > > > > > 
> > > > > > > *I am *not* entirely sure....
> > > > > > 
> > > > > > Hello, thanks for the comments!
> > > > > > The problem is that BuildChangeEndianness only concern about LoadMem
> and
> > > > > > StoreMem but Get/SetGlobal which assumes native  endianness, doesn't
> > call
> > > > > > BuildChangeEndianness.
> > > > > 
> > > > > Oh. In that case we need to deal with the two separately? As it is
right
> > > now,
> > > > it
> > > > > seems  incorrect for non global case.
> > > > 
> > > > Hello, I think simd lowering is not only for wasm but also for JS in
> > general,
> > > > which should respect native endianess, right? In wasm case, non global
> case
> > > > (little endian) will go through LoadMem/StoreMem, which does the swap
for
> > the
> > > > lanes, So lanes numbering would be the same as local order (big endian).
> > > 
> > > Currently, the proposal to add SIMD to JS has been dropped. So, we don't
> need
> > to
> > > worry about non WASM case.
> > 
> > Hello, thanks for the comments.
> > For wasm, we still need to differentiate global and non global case. So I
> think
> > for now, wasm-compiler will change LE to native endian and leave simd
lowering
> > to respect native endian. Is that ok?
> > 
> > Thanks,
> > John
> 
> Yeah. Seems like that should work. Do test it before submitting.

Thanks! I have tested on both LE and BE systems.

[john.yan, 2017-05-05 18:41:15]

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

[john.yan, 2017-05-05 18:41:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
clemensh@chromium.org
Link to the patchset: https://codereview.chromium.org/2838943002/#ps380001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-05 18:41:26]

CQ is trying da patch.

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

[john.yan, 2017-05-05 18:44:02]

On 2017/05/04 21:35:23, aseemgarg wrote:
> On 2017/05/04 18:50:55, john.yan wrote:
> > On 2017/05/03 22:03:53, aseemgarg wrote:
> > > On 2017/05/03 17:53:35, john.yan wrote:
> > > > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > > > I am entirely sure if the scalar-lowering part is needed. As I
> > > > understand,
> > > > > > the
> > > > > > > > BuildChangeEndianness will be called at the time the graph is
> built.
> > > In
> > > > > that
> > > > > > > > case, at the time of lowering we shouldn't need to once again
> > consider
> > > > the
> > > > > > > > indices.
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > src/compiler/simd-scalar-lowering.cc:237:
> > > rep_nodes[0]->ReplaceInput(1,
> > > > > > > > indices[0]);
> > > > > > > > This is not correct. The else case (without control and effect
> > nodes)
> > > is
> > > > > not
> > > > > > > > covered. Also, this needs to be rebased. The code has changed
> > somewhat
> > > > > over
> > > > > > > here
> > > > > > > > (Int16x8 was added).
> > > > > > > 
> > > > > > > *I am *not* entirely sure....
> > > > > > 
> > > > > > Hello, thanks for the comments!
> > > > > > The problem is that BuildChangeEndianness only concern about LoadMem
> and
> > > > > > StoreMem but Get/SetGlobal which assumes native  endianness, doesn't
> > call
> > > > > > BuildChangeEndianness.
> > > > > 
> > > > > Oh. In that case we need to deal with the two separately? As it is
right
> > > now,
> > > > it
> > > > > seems  incorrect for non global case.
> > > > 
> > > > Hello, I think simd lowering is not only for wasm but also for JS in
> > general,
> > > > which should respect native endianess, right? In wasm case, non global
> case
> > > > (little endian) will go through LoadMem/StoreMem, which does the swap
for
> > the
> > > > lanes, So lanes numbering would be the same as local order (big endian).
> > > 
> > > Currently, the proposal to add SIMD to JS has been dropped. So, we don't
> need
> > to
> > > worry about non WASM case.
> > 
> > Hello, thanks for the comments.
> > For wasm, we still need to differentiate global and non global case. So I
> think
> > for now, wasm-compiler will change LE to native endian and leave simd
lowering
> > to respect native endian. Is that ok?
> > 
> > Thanks,
> > John
> 
> Yeah. Seems like that should work. Do test it before submitting.

Could you please lgtm on simd-lowering files?
Thanks!

[commit-bot: I haz the power, 2017-05-05 18:44:35]

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

[commit-bot: I haz the power, 2017-05-05 18:44:37]

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/40577)

[john.yan, 2017-05-09 17:11:11]

On 2017/05/05 18:44:02, john.yan wrote:
> On 2017/05/04 21:35:23, aseemgarg wrote:
> > On 2017/05/04 18:50:55, john.yan wrote:
> > > On 2017/05/03 22:03:53, aseemgarg wrote:
> > > > On 2017/05/03 17:53:35, john.yan wrote:
> > > > > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > > > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > > > > I am entirely sure if the scalar-lowering part is needed. As I
> > > > > understand,
> > > > > > > the
> > > > > > > > > BuildChangeEndianness will be called at the time the graph is
> > built.
> > > > In
> > > > > > that
> > > > > > > > > case, at the time of lowering we shouldn't need to once again
> > > consider
> > > > > the
> > > > > > > > > indices.
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > > src/compiler/simd-scalar-lowering.cc:237:
> > > > rep_nodes[0]->ReplaceInput(1,
> > > > > > > > > indices[0]);
> > > > > > > > > This is not correct. The else case (without control and effect
> > > nodes)
> > > > is
> > > > > > not
> > > > > > > > > covered. Also, this needs to be rebased. The code has changed
> > > somewhat
> > > > > > over
> > > > > > > > here
> > > > > > > > > (Int16x8 was added).
> > > > > > > > 
> > > > > > > > *I am *not* entirely sure....
> > > > > > > 
> > > > > > > Hello, thanks for the comments!
> > > > > > > The problem is that BuildChangeEndianness only concern about
LoadMem
> > and
> > > > > > > StoreMem but Get/SetGlobal which assumes native  endianness,
doesn't
> > > call
> > > > > > > BuildChangeEndianness.
> > > > > > 
> > > > > > Oh. In that case we need to deal with the two separately? As it is
> right
> > > > now,
> > > > > it
> > > > > > seems  incorrect for non global case.
> > > > > 
> > > > > Hello, I think simd lowering is not only for wasm but also for JS in
> > > general,
> > > > > which should respect native endianess, right? In wasm case, non global
> > case
> > > > > (little endian) will go through LoadMem/StoreMem, which does the swap
> for
> > > the
> > > > > lanes, So lanes numbering would be the same as local order (big
endian).
> > > > 
> > > > Currently, the proposal to add SIMD to JS has been dropped. So, we don't
> > need
> > > to
> > > > worry about non WASM case.
> > > 
> > > Hello, thanks for the comments.
> > > For wasm, we still need to differentiate global and non global case. So I
> > think
> > > for now, wasm-compiler will change LE to native endian and leave simd
> lowering
> > > to respect native endian. Is that ok?
> > > 
> > > Thanks,
> > > John
> > 
> > Yeah. Seems like that should work. Do test it before submitting.
> 
> Could you please lgtm on simd-lowering files?
> Thanks!

Just a reminder.

Thanks,
John

[aseemgarg, 2017-05-09 18:32:34]

On 2017/05/09 17:11:11, john.yan wrote:
> On 2017/05/05 18:44:02, john.yan wrote:
> > On 2017/05/04 21:35:23, aseemgarg wrote:
> > > On 2017/05/04 18:50:55, john.yan wrote:
> > > > On 2017/05/03 22:03:53, aseemgarg wrote:
> > > > > On 2017/05/03 17:53:35, john.yan wrote:
> > > > > > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > > > > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > > > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > > > > > I am entirely sure if the scalar-lowering part is needed. As
I
> > > > > > understand,
> > > > > > > > the
> > > > > > > > > > BuildChangeEndianness will be called at the time the graph
is
> > > built.
> > > > > In
> > > > > > > that
> > > > > > > > > > case, at the time of lowering we shouldn't need to once
again
> > > > consider
> > > > > > the
> > > > > > > > > > indices.
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > > > src/compiler/simd-scalar-lowering.cc:237:
> > > > > rep_nodes[0]->ReplaceInput(1,
> > > > > > > > > > indices[0]);
> > > > > > > > > > This is not correct. The else case (without control and
effect
> > > > nodes)
> > > > > is
> > > > > > > not
> > > > > > > > > > covered. Also, this needs to be rebased. The code has
changed
> > > > somewhat
> > > > > > > over
> > > > > > > > > here
> > > > > > > > > > (Int16x8 was added).
> > > > > > > > > 
> > > > > > > > > *I am *not* entirely sure....
> > > > > > > > 
> > > > > > > > Hello, thanks for the comments!
> > > > > > > > The problem is that BuildChangeEndianness only concern about
> LoadMem
> > > and
> > > > > > > > StoreMem but Get/SetGlobal which assumes native  endianness,
> doesn't
> > > > call
> > > > > > > > BuildChangeEndianness.
> > > > > > > 
> > > > > > > Oh. In that case we need to deal with the two separately? As it is
> > right
> > > > > now,
> > > > > > it
> > > > > > > seems  incorrect for non global case.
> > > > > > 
> > > > > > Hello, I think simd lowering is not only for wasm but also for JS in
> > > > general,
> > > > > > which should respect native endianess, right? In wasm case, non
global
> > > case
> > > > > > (little endian) will go through LoadMem/StoreMem, which does the
swap
> > for
> > > > the
> > > > > > lanes, So lanes numbering would be the same as local order (big
> endian).
> > > > > 
> > > > > Currently, the proposal to add SIMD to JS has been dropped. So, we
don't
> > > need
> > > > to
> > > > > worry about non WASM case.
> > > > 
> > > > Hello, thanks for the comments.
> > > > For wasm, we still need to differentiate global and non global case. So
I
> > > think
> > > > for now, wasm-compiler will change LE to native endian and leave simd
> > lowering
> > > > to respect native endian. Is that ok?
> > > > 
> > > > Thanks,
> > > > John
> > > 
> > > Yeah. Seems like that should work. Do test it before submitting.
> > 
> > Could you please lgtm on simd-lowering files?
> > Thanks!
> 
> Just a reminder.
> 
> Thanks,
> John

lgtm. Someone in src/compiler/OWNERS needs to lgtm

[Jarin, 2017-05-09 18:55:20]

lgtm (rubber-stamp).

[john.yan, 2017-05-09 19:16:57]

On 2017/05/09 18:32:34, aseemgarg wrote:
> On 2017/05/09 17:11:11, john.yan wrote:
> > On 2017/05/05 18:44:02, john.yan wrote:
> > > On 2017/05/04 21:35:23, aseemgarg wrote:
> > > > On 2017/05/04 18:50:55, john.yan wrote:
> > > > > On 2017/05/03 22:03:53, aseemgarg wrote:
> > > > > > On 2017/05/03 17:53:35, john.yan wrote:
> > > > > > > On 2017/05/02 21:55:43, aseemgarg wrote:
> > > > > > > > On 2017/05/02 21:40:46, john.yan wrote:
> > > > > > > > > On 2017/05/02 21:20:59, aseemgarg wrote:
> > > > > > > > > > On 2017/05/02 21:20:21, aseemgarg wrote:
> > > > > > > > > > > I am entirely sure if the scalar-lowering part is needed.
As
> I
> > > > > > > understand,
> > > > > > > > > the
> > > > > > > > > > > BuildChangeEndianness will be called at the time the graph
> is
> > > > built.
> > > > > > In
> > > > > > > > that
> > > > > > > > > > > case, at the time of lowering we shouldn't need to once
> again
> > > > > consider
> > > > > > > the
> > > > > > > > > > > indices.
> > > > > > > > > > > 
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > > > > File src/compiler/simd-scalar-lowering.cc (right):
> > > > > > > > > > > 
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2838943002/diff/280001/src/compiler/simd-scal...
> > > > > > > > > > > src/compiler/simd-scalar-lowering.cc:237:
> > > > > > rep_nodes[0]->ReplaceInput(1,
> > > > > > > > > > > indices[0]);
> > > > > > > > > > > This is not correct. The else case (without control and
> effect
> > > > > nodes)
> > > > > > is
> > > > > > > > not
> > > > > > > > > > > covered. Also, this needs to be rebased. The code has
> changed
> > > > > somewhat
> > > > > > > > over
> > > > > > > > > > here
> > > > > > > > > > > (Int16x8 was added).
> > > > > > > > > > 
> > > > > > > > > > *I am *not* entirely sure....
> > > > > > > > > 
> > > > > > > > > Hello, thanks for the comments!
> > > > > > > > > The problem is that BuildChangeEndianness only concern about
> > LoadMem
> > > > and
> > > > > > > > > StoreMem but Get/SetGlobal which assumes native  endianness,
> > doesn't
> > > > > call
> > > > > > > > > BuildChangeEndianness.
> > > > > > > > 
> > > > > > > > Oh. In that case we need to deal with the two separately? As it
is
> > > right
> > > > > > now,
> > > > > > > it
> > > > > > > > seems  incorrect for non global case.
> > > > > > > 
> > > > > > > Hello, I think simd lowering is not only for wasm but also for JS
in
> > > > > general,
> > > > > > > which should respect native endianess, right? In wasm case, non
> global
> > > > case
> > > > > > > (little endian) will go through LoadMem/StoreMem, which does the
> swap
> > > for
> > > > > the
> > > > > > > lanes, So lanes numbering would be the same as local order (big
> > endian).
> > > > > > 
> > > > > > Currently, the proposal to add SIMD to JS has been dropped. So, we
> don't
> > > > need
> > > > > to
> > > > > > worry about non WASM case.
> > > > > 
> > > > > Hello, thanks for the comments.
> > > > > For wasm, we still need to differentiate global and non global case.
So
> I
> > > > think
> > > > > for now, wasm-compiler will change LE to native endian and leave simd
> > > lowering
> > > > > to respect native endian. Is that ok?
> > > > > 
> > > > > Thanks,
> > > > > John
> > > > 
> > > > Yeah. Seems like that should work. Do test it before submitting.
> > > 
> > > Could you please lgtm on simd-lowering files?
> > > Thanks!
> > 
> > Just a reminder.
> > 
> > Thanks,
> > John
> 
> lgtm. Someone in src/compiler/OWNERS needs to lgtm

thanks!

[john.yan, 2017-05-09 19:17:11]

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

[commit-bot: I haz the power, 2017-05-09 19:17:15]

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, 2017-05-09 19:54:20]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1494357431577390,
"parent_rev": "266ff75630ea0d2ea901fdc461089a529222f859", "commit_rev":
"18c33c504a9ba61df2fa3833210f83539d2569f4"}

[commit-bot: I haz the power, 2017-05-09 19:54:29]

Description was changed from

==========
[wasm] Implement 128-bit endian swap for simd type

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

to

==========
[wasm] Implement 128-bit endian swap for simd type

BUG=

Review-Url: https://codereview.chromium.org/2838943002
Cr-Commit-Position: refs/heads/master@{#45208}
Committed:
https://chromium.googlesource.com/v8/v8/+/18c33c504a9ba61df2fa3833210f83539d2...
==========

[commit-bot: I haz the power, 2017-05-09 19:54:30]

Committed patchset #19 (id:380001) as
https://chromium.googlesource.com/v8/v8/+/18c33c504a9ba61df2fa3833210f83539d2...