Add several SIMD opcodes to IA32

Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd, psubd, vpaddd, vpsubd, pinsrw to ia32-assembler

BUG=

Review-Url: https://codereview.chromium.org/2695613004
Cr-Original-Commit-Position: refs/heads/master@{#43483}
Committed: https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a80a15
Review-Url: https://codereview.chromium.org/2695613004
Cr-Commit-Position: refs/heads/master@{#43708}
Committed: https://chromium.googlesource.com/v8/v8/+/b9614d4bd1fc7a6a32a7bcc33e793e2d7e520c59

[Jing, 2017-02-15 08:43:23]

jing.bao@intel.com changed reviewers:
+ bbudge@chromium.org, gdeepti@chromium.org

[bbudge, 2017-02-15 16:00:12]

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:985: byte opcode);
This should probably be either private, or even better, a static function
defined in assembler-ia32.cc. You don't want clients of assembler to call it.

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:994: SSE2_INSTRUCTION_LIST(DECLARE_SSE2_INSTRUCTION)
I think it would be more readable if SSE2_INSTRUCTION_LIST was defined in this
file, rather than in a separate header.

https://codereview.chromium.org/2695613004/diff/20001/test/cctest/BUILD.gn
File test/cctest/BUILD.gn (right):

https://codereview.chromium.org/2695613004/diff/20001/test/cctest/BUILD.gn#ne...
test/cctest/BUILD.gn:241: "wasm/test-run-wasm-simd.cc",
Since you're switching to the native tests, you should add #ifdef's in this file
to run the tests for the opcodes you implement in this CL. These should be
similar to the #ifdef's already there for x64.

[bbudge, 2017-02-15 16:07:31]

A few more comments.

https://codereview.chromium.org/2695613004/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1921: __
paddd(i.OutputSimd128Register(), i.InputSimd128Register(1));
If it's planned to use AVX, then we should make these macro instructions and use
the macroassembler to switch between implementations.

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:985: byte opcode);
On 2017/02/15 16:00:12, bbudge wrote:
> This should probably be either private, or even better, a static function
> defined in assembler-ia32.cc. You don't want clients of assembler to call it.

Looking at the implementation, this has to be a class method, so make it
protected or private.

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:1101: // AVX instructions
Is there a plan to use AVX if it's available?

[Jing, 2017-02-16 05:11:17]

In addition to the comments, I also change shufps to pshufd since it's for
Int32x4 type.

https://codereview.chromium.org/2695613004/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1921: __
paddd(i.OutputSimd128Register(), i.InputSimd128Register(1));
On 2017/02/15 16:07:31, bbudge wrote:
> If it's planned to use AVX, then we should make these macro instructions and
use
> the macroassembler to switch between implementations.

To add AVX version ,we do have some code needed to add/change. For example, all
other movds should also be changed to Movds.
I plan to supplement the SSE instructions as well as their AVX version in next
patch. Is it OK to change the instructions to macro instructions at that time?

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:985: byte opcode);
On 2017/02/15 16:07:31, bbudge wrote:
> On 2017/02/15 16:00:12, bbudge wrote:
> > This should probably be either private, or even better, a static function
> > defined in assembler-ia32.cc. You don't want clients of assembler to call
it.
> 
> Looking at the implementation, this has to be a class method, so make it
> protected or private.

Done.

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:994: SSE2_INSTRUCTION_LIST(DECLARE_SSE2_INSTRUCTION)
On 2017/02/15 16:00:12, bbudge wrote:
> I think it would be more readable if SSE2_INSTRUCTION_LIST was defined in this
> file, rather than in a separate header.

I ported the header file from x64 and planned to expand the instruction list to
at least as many instructions as x64.
https://chromium.googlesource.com/v8/v8.git/+/master/src/x64/sse-instr.h
I don't know which is preferred? To add the list in current file or add a new
header file?

https://codereview.chromium.org/2695613004/diff/20001/test/cctest/BUILD.gn
File test/cctest/BUILD.gn (right):

https://codereview.chromium.org/2695613004/diff/20001/test/cctest/BUILD.gn#ne...
test/cctest/BUILD.gn:241: "wasm/test-run-wasm-simd.cc",
On 2017/02/15 16:00:12, bbudge wrote:
> Since you're switching to the native tests, you should add #ifdef's in this
file
> to run the tests for the opcodes you implement in this CL. These should be
> similar to the #ifdef's already there for x64.

In the file tests for my implemented opcodes are by-default on, so there's no
need to add #ifdef's this time. I will remember to add them when I add opcodes
in the future.

[bbudge, 2017-02-17 00:44:40]

https://codereview.chromium.org/2695613004/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1921: __
paddd(i.OutputSimd128Register(), i.InputSimd128Register(1));
On 2017/02/16 05:11:17, Jing wrote:
> On 2017/02/15 16:07:31, bbudge wrote:
> > If it's planned to use AVX, then we should make these macro instructions and
> use
> > the macroassembler to switch between implementations.
> 
> To add AVX version ,we do have some code needed to add/change. For example,
all
> other movds should also be changed to Movds.
> I plan to supplement the SSE instructions as well as their AVX version in next
> patch. Is it OK to change the instructions to macro instructions at that time?

OK.

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:994: SSE2_INSTRUCTION_LIST(DECLARE_SSE2_INSTRUCTION)
On 2017/02/16 05:11:17, Jing wrote:
> On 2017/02/15 16:00:12, bbudge wrote:
> > I think it would be more readable if SSE2_INSTRUCTION_LIST was defined in
this
> > file, rather than in a separate header.
> 
> I ported the header file from x64 and planned to expand the instruction list
to
> at least as many instructions as x64.
> https://chromium.googlesource.com/v8/v8.git/+/master/src/x64/sse-instr.h
> I don't know which is preferred? To add the list in current file or add a new
> header file?

I think it's more consistent with V8 style to inline the list here.

https://codereview.chromium.org/2695613004/diff/20001/test/cctest/BUILD.gn
File test/cctest/BUILD.gn (right):

https://codereview.chromium.org/2695613004/diff/20001/test/cctest/BUILD.gn#ne...
test/cctest/BUILD.gn:241: "wasm/test-run-wasm-simd.cc",
On 2017/02/16 05:11:17, Jing wrote:
> On 2017/02/15 16:00:12, bbudge wrote:
> > Since you're switching to the native tests, you should add #ifdef's in this
> file
> > to run the tests for the opcodes you implement in this CL. These should be
> > similar to the #ifdef's already there for x64.
> 
> In the file tests for my implemented opcodes are by-default on, so there's no
> need to add #ifdef's this time. I will remember to add them when I add opcodes
> in the future.

That's right. OK

[Jing, 2017-02-17 03:51:35]

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:994: SSE2_INSTRUCTION_LIST(DECLARE_SSE2_INSTRUCTION)
On 2017/02/17 00:44:40, bbudge wrote:
> On 2017/02/16 05:11:17, Jing wrote:
> > On 2017/02/15 16:00:12, bbudge wrote:
> > > I think it would be more readable if SSE2_INSTRUCTION_LIST was defined in
> this
> > > file, rather than in a separate header.
> > 
> > I ported the header file from x64 and planned to expand the instruction list
> to
> > at least as many instructions as x64.
> > https://chromium.googlesource.com/v8/v8.git/+/master/src/x64/sse-instr.h
> > I don't know which is preferred? To add the list in current file or add a
new
> > header file?
> 
> I think it's more consistent with V8 style to inline the list here.

Done.

[bbudge, 2017-02-18 00:02:25]

Just a few comments, otherwise LGTM

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1696: Emit(kIA32Int32x4Create,
g.DefineAsRegister(node), g.Use(node->InputAt(0)));
Call VisitRO(this, node, kIA32Int32x4Create)

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1717:
g.UseRegister(node->InputAt(0)), g.UseRegister(node->InputAt(1)));
If there will be a lot of similar methods here, it would be good to factor out
the common code into a new VisitRRR function at the top of this file, and call
that in this method and the one below.

https://codereview.chromium.org/2695613004/diff/60001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/60001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:1430: sse2_instr(dst, src, 0x##prefix, 0x##escape,
0x##opcode);         \
Rather than gluing the literals together here, could you declare them above?
I.e.

#define SSE2_INSTRUCTION_LIST(V) \
  V(paddd, 0x66, 0x0F, 0xFE)           \
  V(psubd, 0x66, 0x0F, 0xFA)

I think it's clearer and more consistent with style here.

[Jing, 2017-02-20 15:28:13]

Hi Bill,
I'm quite sorry that I rebased my local branch and don't know how to undo that.
Also I added AVX version of add/sub to explain something more clearly and revert
patch set 4 for some reason. So you may have to take a look again.

Sorry for the big change...

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:994: SSE2_INSTRUCTION_LIST(DECLARE_SSE2_INSTRUCTION)
On 2017/02/17 03:51:35, Jing wrote:
> On 2017/02/17 00:44:40, bbudge wrote:
> > On 2017/02/16 05:11:17, Jing wrote:
> > > On 2017/02/15 16:00:12, bbudge wrote:
> > > > I think it would be more readable if SSE2_INSTRUCTION_LIST was defined
in
> > this
> > > > file, rather than in a separate header.
> > > 
> > > I ported the header file from x64 and planned to expand the instruction
list
> > to
> > > at least as many instructions as x64.
> > > https://chromium.googlesource.com/v8/v8.git/+/master/src/x64/sse-instr.h
> > > I don't know which is preferred? To add the list in current file or add a
> new
> > > header file?
> > 
> > I think it's more consistent with V8 style to inline the list here.
> 
> Done.

Sorry, I recover the separate header file again, because it has to be included
in disasm-ia32.cc

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1696: Emit(kIA32Int32x4Create,
g.DefineAsRegister(node), g.Use(node->InputAt(0)));
On 2017/02/18 00:02:25, bbudge wrote:
> Call VisitRO(this, node, kIA32Int32x4Create)

Done.

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1717:
g.UseRegister(node->InputAt(0)), g.UseRegister(node->InputAt(1)));
On 2017/02/18 00:02:25, bbudge wrote:
> If there will be a lot of similar methods here, it would be good to factor out
> the common code into a new VisitRRR function at the top of this file, and call
> that in this method and the one below.

Actually I found that g.UseRegister(node->InputAt(1)) is too narrow here. We can
write g.Use(). Sorry for my thoughtless porting from x64 ...

Int32x4Add can be implemented by "PADDD xmm1, xmm2/m128" or "VPADDD xmm1, xmm2,
xmm3/m128", and they match VisitRROFloat
(https://chromium.googlesource.com/v8/v8.git/+/master/src/compiler/ia32/instru...).
So I guess the best way is to put it in RRO_FLOAT_OP_LIST, and add the AVX
version in this patch?

I did similar change on Int32x4Sub. AVX version of other SIMD opcodes will be
added in later patches.

https://codereview.chromium.org/2695613004/diff/60001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/60001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:1430: sse2_instr(dst, src, 0x##prefix, 0x##escape,
0x##opcode);         \
On 2017/02/18 00:02:25, bbudge wrote:
> Rather than gluing the literals together here, could you declare them above?
> I.e.
> 
> #define SSE2_INSTRUCTION_LIST(V) \
>   V(paddd, 0x66, 0x0F, 0xFE)           \
>   V(psubd, 0x66, 0x0F, 0xFA)
> 
> I think it's clearer and more consistent with style here.

Since the list is also used in DECLARE_SSE2_AVX_INSTRUCTION, where I use k66 and
k0F, is it OK to remain the literal gluing?

[bbudge, 2017-02-22 21:51:10]

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/20001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:994: SSE2_INSTRUCTION_LIST(DECLARE_SSE2_INSTRUCTION)
On 2017/02/20 15:28:13, Jing wrote:
> On 2017/02/17 03:51:35, Jing wrote:
> > On 2017/02/17 00:44:40, bbudge wrote:
> > > On 2017/02/16 05:11:17, Jing wrote:
> > > > On 2017/02/15 16:00:12, bbudge wrote:
> > > > > I think it would be more readable if SSE2_INSTRUCTION_LIST was defined
> in
> > > this
> > > > > file, rather than in a separate header.
> > > > 
> > > > I ported the header file from x64 and planned to expand the instruction
> list
> > > to
> > > > at least as many instructions as x64.
> > > > https://chromium.googlesource.com/v8/v8.git/+/master/src/x64/sse-instr.h
> > > > I don't know which is preferred? To add the list in current file or add
a
> > new
> > > > header file?
> > > 
> > > I think it's more consistent with V8 style to inline the list here.
> > 
> > Done.
> 
> Sorry, I recover the separate header file again, because it has to be included
> in disasm-ia32.cc

OK, I see the precedent in x64.

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/60001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1717:
g.UseRegister(node->InputAt(0)), g.UseRegister(node->InputAt(1)));
On 2017/02/20 15:28:13, Jing wrote:
> On 2017/02/18 00:02:25, bbudge wrote:
> > If there will be a lot of similar methods here, it would be good to factor
out
> > the common code into a new VisitRRR function at the top of this file, and
call
> > that in this method and the one below.
> 
> Actually I found that g.UseRegister(node->InputAt(1)) is too narrow here. We
can
> write g.Use(). Sorry for my thoughtless porting from x64 ...
> 
> Int32x4Add can be implemented by "PADDD xmm1, xmm2/m128" or "VPADDD xmm1,
xmm2,
> xmm3/m128", and they match VisitRROFloat
>
(https://chromium.googlesource.com/v8/v8.git/+/master/src/compiler/ia32/instru...).
> So I guess the best way is to put it in RRO_FLOAT_OP_LIST, and add the AVX
> version in this patch?
> 
> I did similar change on Int32x4Sub. AVX version of other SIMD opcodes will be
> added in later patches.

Thanks!

https://codereview.chromium.org/2695613004/diff/60001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/2695613004/diff/60001/src/ia32/assembler-ia32...
src/ia32/assembler-ia32.h:1430: sse2_instr(dst, src, 0x##prefix, 0x##escape,
0x##opcode);         \
On 2017/02/20 15:28:13, Jing wrote:
> On 2017/02/18 00:02:25, bbudge wrote:
> > Rather than gluing the literals together here, could you declare them above?
> > I.e.
> > 
> > #define SSE2_INSTRUCTION_LIST(V) \
> >   V(paddd, 0x66, 0x0F, 0xFE)           \
> >   V(psubd, 0x66, 0x0F, 0xFA)
> > 
> > I think it's clearer and more consistent with style here.
> 
> Since the list is also used in DECLARE_SSE2_AVX_INSTRUCTION, where I use k66
and
> k0F, is it OK to remain the literal gluing?

In that case, this is OK.

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1916: CpuFeatureScope sse_scope(masm(),
SSE4_1);
This scope is taken by the macro assembler function.

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1921: CpuFeatureScope sse_scope(masm(),
SSE4_1);
Ditto

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1925: case kSSEInt32x4Add: {
SSE scope?

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1929: case kSSEInt32x4Sub: {
Ditto

[Jing, 2017-02-23 07:28:17]

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1916: CpuFeatureScope sse_scope(masm(),
SSE4_1);
On 2017/02/22 21:51:10, bbudge wrote:
> This scope is taken by the macro assembler function.

Done.
I adjusted Pextrd macro to support non-SSE4 occasion.

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1921: CpuFeatureScope sse_scope(masm(),
SSE4_1);
On 2017/02/22 21:51:10, bbudge wrote:
> Ditto

I cannot remove the SSE4 check here, because it will cause error on non-SSE4
hardware. To avoid redundant scope, I change Pinsrd macro to raw pinsrd.

When SSE4 is not supported, Pinsrd will use some SSE2 instructions to handle
specific cases for kSSEFloat64InsertLowWord32 and kSSEFloat64InsertHighWord32.
But the specific path is wrong for the semantic meaning of "pinsrd", because it
doesn't care about the high 64 bits of xmm register.

I haven't find a proper way to emulate non-SSE4 Pinsrd similar to Pextrd,and I'm
still seeking. Do you have any suggestions?

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1925: case kSSEInt32x4Add: {
On 2017/02/22 21:51:10, bbudge wrote:
> SSE scope?

paddd and psubd are both SSE2 instructions. I guess that they are assumed on by
default and need no scope?

[bbudge, 2017-02-24 18:28:54]

Still LGTM. You may need a V8 compiler owner to land this.

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1921: CpuFeatureScope sse_scope(masm(),
SSE4_1);
On 2017/02/23 07:28:16, Jing wrote:
> On 2017/02/22 21:51:10, bbudge wrote:
> > Ditto
> 
> I cannot remove the SSE4 check here, because it will cause error on non-SSE4
> hardware. To avoid redundant scope, I change Pinsrd macro to raw pinsrd.
> 
> When SSE4 is not supported, Pinsrd will use some SSE2 instructions to handle
> specific cases for kSSEFloat64InsertLowWord32 and kSSEFloat64InsertHighWord32.
> But the specific path is wrong for the semantic meaning of "pinsrd", because
it
> doesn't care about the high 64 bits of xmm register.
> 
> I haven't find a proper way to emulate non-SSE4 Pinsrd similar to Pextrd,and
I'm
> still seeking. Do you have any suggestions?

I don't have any suggestions. I've found googling effective to find the best
SIMD code sequences for various operations. I'm worried about performance issues
from mixing the various instructions together. I think we've had problems mixing
SSE and AVX.

https://codereview.chromium.org/2695613004/diff/80001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:1925: case kSSEInt32x4Add: {
On 2017/02/23 07:28:16, Jing wrote:
> On 2017/02/22 21:51:10, bbudge wrote:
> > SSE scope?
> 
> paddd and psubd are both SSE2 instructions. I guess that they are assumed on
by
> default and need no scope?

OK

https://codereview.chromium.org/2695613004/diff/100001/src/compiler/ia32/code...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/100001/src/compiler/ia32/code...
src/compiler/ia32/code-generator-ia32.cc:1909: case kIA32Int32x4Create: {
Could you rename the opcode kIA32Int32x4Splat? I'm eliminating the Create ops
since they're really splats.

[Jing, 2017-02-25 02:49:04]

jing.bao@intel.com changed reviewers:
+ bmeurer@chromium.org

[Jing, 2017-02-25 02:49:04]

Adding Benedikt as reviewer.

https://codereview.chromium.org/2695613004/diff/100001/src/compiler/ia32/code...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2695613004/diff/100001/src/compiler/ia32/code...
src/compiler/ia32/code-generator-ia32.cc:1909: case kIA32Int32x4Create: {
On 2017/02/24 18:28:54, bbudge wrote:
> Could you rename the opcode kIA32Int32x4Splat? I'm eliminating the Create ops
> since they're really splats.

Done.

[Jing, 2017-02-28 08:52:05]

Hi Benedikt,
please take a look. Thanks!

[Benedikt Meurer, 2017-02-28 08:54:18]

LGTM (rubber-stamped)

[Jing, 2017-02-28 09:05:00]

The CQ bit was checked by jing.bao@intel.com

[Jing, 2017-02-28 09:05:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2695613004/#ps140001
(title: "Rebase again")

[commit-bot: I haz the power, 2017-02-28 09:05:03]

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-02-28 09:06:13]

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

[commit-bot: I haz the power, 2017-02-28 09:06:14]

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/21732)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18206)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35645)

[Jing, 2017-02-28 13:11:22]

The CQ bit was checked by jing.bao@intel.com

[Jing, 2017-02-28 13:11:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2695613004/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-28 13:11:29]

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-02-28 13:35:23]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488287482907320,
"parent_rev": "63afdb007534323565d28f8cede64fc50be07ed6", "commit_rev":
"4deb9ffdecf121c69a3db7eae6698eae23a80a15"}

[commit-bot: I haz the power, 2017-02-28 13:35:27]

Description was changed from

==========
Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd and psubd to ia32-assembler

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

to

==========
Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd and psubd to ia32-assembler

BUG=

Review-Url: https://codereview.chromium.org/2695613004
Cr-Commit-Position: refs/heads/master@{#43483}
Committed:
https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a...
==========

[commit-bot: I haz the power, 2017-02-28 13:35:29]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a...

[Michael Achenbach, 2017-02-28 14:54:12]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2717423003/ by machenbach@chromium.org.

The reason for reverting is: Fails with nosse4:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug/builds....

[bbudge, 2017-02-28 18:11:35]

On 2017/02/28 14:54:12, Michael Achenbach wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/2717423003/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: Fails with nosse4:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug/builds....

My guess is that you can't assume SSE on ia32, so you'll have to feature check
in assembler-ia32-inl.h.

[Jing, 2017-03-01 13:18:49]

On 2017/02/28 18:11:35, bbudge wrote:
> On 2017/02/28 14:54:12, Michael Achenbach wrote:
> > A revert of this CL (patchset #9 id:160001) has been created in
> > https://codereview.chromium.org/2717423003/ by
mailto:machenbach@chromium.org.
> > 
> > The reason for reverting is: Fails with nosse4:
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug/builds....
> 
> My guess is that you can't assume SSE on ia32, so you'll have to feature check
> in assembler-ia32-inl.h.

It seems that the nosse4 failure is caused by the SSE4_1 scope for
kIA32Int32x4ReplaceLane.
Because the patch is closed, I uploaded another CL to adjust Pinsrd macro.
After landing that patch, using Pinsrd for kIA32Int32x4ReplaceLane should fix
the failure.

[bbudge, 2017-03-01 16:50:56]

On 2017/03/01 13:18:49, Jing wrote:
> On 2017/02/28 18:11:35, bbudge wrote:
> > On 2017/02/28 14:54:12, Michael Achenbach wrote:
> > > A revert of this CL (patchset #9 id:160001) has been created in
> > > https://codereview.chromium.org/2717423003/ by
> mailto:machenbach@chromium.org.
> > > 
> > > The reason for reverting is: Fails with nosse4:
> > >
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug/builds....
> > 
> > My guess is that you can't assume SSE on ia32, so you'll have to feature
check
> > in assembler-ia32-inl.h.
> 
> It seems that the nosse4 failure is caused by the SSE4_1 scope for
> kIA32Int32x4ReplaceLane.
> Because the patch is closed, I uploaded another CL to adjust Pinsrd macro.
> After landing that patch, using Pinsrd for kIA32Int32x4ReplaceLane should fix
> the failure.

Please reopen by unchecking the 'closed' checkbox so we have this review
history.

[Jing, 2017-03-02 04:57:08]

Description was changed from

==========
Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd and psubd to ia32-assembler

BUG=

Review-Url: https://codereview.chromium.org/2695613004
Cr-Commit-Position: refs/heads/master@{#43483}
Committed:
https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a...
==========

to

==========
Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd, psubd, vpaddd, vpsubd, pinsrw to ia32-assembler

BUG=

Review-Url: https://codereview.chromium.org/2695613004
Cr-Commit-Position: refs/heads/master@{#43483}
Committed:
https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a...
==========

[Jing, 2017-03-02 05:08:32]

On 2017/03/01 16:50:56, bbudge wrote:
> On 2017/03/01 13:18:49, Jing wrote:
> > On 2017/02/28 18:11:35, bbudge wrote:
> > > On 2017/02/28 14:54:12, Michael Achenbach wrote:
> > > > A revert of this CL (patchset #9 id:160001) has been created in
> > > > https://codereview.chromium.org/2717423003/ by
> > mailto:machenbach@chromium.org.
> > > > 
> > > > The reason for reverting is: Fails with nosse4:
> > > >
> > >
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug/builds....
> > > 
> > > My guess is that you can't assume SSE on ia32, so you'll have to feature
> check
> > > in assembler-ia32-inl.h.
> > 
> > It seems that the nosse4 failure is caused by the SSE4_1 scope for
> > kIA32Int32x4ReplaceLane.
> > Because the patch is closed, I uploaded another CL to adjust Pinsrd macro.
> > After landing that patch, using Pinsrd for kIA32Int32x4ReplaceLane should
fix
> > the failure.
> 
> Please reopen by unchecking the 'closed' checkbox so we have this review
> history.

I uploaded that CL again to this reopened patch.

Some details on Pinsrd:
When SSE4 is not supported, the original shorter special cases where imm8 = 0, 1
is only for kSSEFloat64InsertXXX, because it will change the high 64 bits of xmm
register. I add a bool to indicate the special case, and use pinsrw otherwise.
(Maybe Benedikt could take a look at this as well as Pextrd because you're the
original author).

[Jing, 2017-03-06 10:38:40]

Ping

[bbudge, 2017-03-06 20:46:12]

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

[bbudge, 2017-03-06 20:46:13]

Still lgtm. It looks like you need a review from a wasm OWNER, adding mtrofin.

[Jing, 2017-03-09 15:13:59]

Ping Trofin.
PTAL, Thanks!

[Mircea Trofin, 2017-03-09 15:25:25]

On 2017/03/09 15:13:59, Jing wrote:
> Ping Trofin.
> PTAL, Thanks!

rubberstamp lgtm

[Jing, 2017-03-10 02:13:04]

The CQ bit was checked by jing.bao@intel.com

[Jing, 2017-03-10 02:13:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2695613004/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-10 02:13:14]

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-03-10 02:40:08]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489111984810620,
"parent_rev": "fd5b3e755df541b44128caed625215017ef59989", "commit_rev":
"b9614d4bd1fc7a6a32a7bcc33e793e2d7e520c59"}

[commit-bot: I haz the power, 2017-03-10 02:40:14]

Description was changed from

==========
Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd, psubd, vpaddd, vpsubd, pinsrw to ia32-assembler

BUG=

Review-Url: https://codereview.chromium.org/2695613004
Cr-Commit-Position: refs/heads/master@{#43483}
Committed:
https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a...
==========

to

==========
Add several SIMD opcodes to IA32

CreateInt32x4, Int32x4ExtractLane, Int32x4ReplaceLane
Int32x4Add, Int32x4Sub

Also add paddd, psubd, vpaddd, vpsubd, pinsrw to ia32-assembler

BUG=

Review-Url: https://codereview.chromium.org/2695613004
Cr-Original-Commit-Position: refs/heads/master@{#43483}
Committed:
https://chromium.googlesource.com/v8/v8/+/4deb9ffdecf121c69a3db7eae6698eae23a...
Review-Url: https://codereview.chromium.org/2695613004
Cr-Commit-Position: refs/heads/master@{#43708}
Committed:
https://chromium.googlesource.com/v8/v8/+/b9614d4bd1fc7a6a32a7bcc33e793e2d7e5...
==========

[commit-bot: I haz the power, 2017-03-10 02:40:16]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/v8/v8/+/b9614d4bd1fc7a6a32a7bcc33e793e2d7e5...