Lower insertelement and extractelement.

Lower insertelement and extractelement.

Use instructions that do the operations in registers and that are
available in SSE2. Spill to memory to perform the operation in the
absence of any other reasonable options (v16i8 and v16i1).

Unfortunately there is no natural class of SSE2 instructions that
insertelement / extractelement can get lowered
to for all vector types (though pinsr[bwd] and pextr[bwd] are
available in SSE4.1). There are in some cases a large number of
choices available for lowering and I have not looked into which
choices are the best yet, besides using LLVM output as a guide.

BUG=none
R=jvoung@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=4988923

[native-client-reviews_googlegroups.com, 2014-07-17 17:51:56]

FYI LLVM is undergoing a pretty big rewrite of its shufflevector lowering
for x86. It may be worth looking at what Chandler did there.


On Wed, Jul 16, 2014 at 6:51 PM, <wala@chromium.org> wrote:

> Reviewers: jvoung, stichnot,
>
> Description:
> Lower insertelement and extractelement.
>
> Use instructions that do the operations in registers and that are
> available in SSE2. Spill to memory to perform the operation when we
> have to (v16i8 and v16i1). Unfortunately there is no natural class of
> SSE2 instructions that insertelement / extractelement can get lowered
> to for all vector types (though pinsr[bwd] and pextr[bwd] are
> available in SSE4.1). There are in some cases a large number of
> choices available for lowering and I have not looked into which
> choices are the best yet.
>
> BUG=none
>
> Please review this at https://codereview.chromium.org/401523003/
>
> SVN Base: https://gerrit.chromium.org/gerrit/p/native_client/pnacl-
> subzero.git@master
>
> Affected files (+1477, -0 lines):
>   M crosstest/runtests.sh
>   A crosstest/test_vector_ops.def
>   A crosstest/test_vector_ops.ll
>   A crosstest/test_vector_ops_main.cpp
>   M src/IceConverter.cpp
>   M src/IceInst.h
>   M src/IceInst.cpp
>   M src/IceInstX8632.h
>   M src/IceInstX8632.cpp
>   M src/IceTargetLowering.h
>   M src/IceTargetLowering.cpp
>   M src/IceTargetLoweringX8632.h
>   M src/IceTargetLoweringX8632.cpp
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to native-client-reviews@
> googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jvoung (off chromium), 2014-07-17 19:36:38]

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp#newcode477
src/IceInstX8632.cpp:477: template <> const char *InstX8632Lea::Opcode = "lea";
Cluster lea, and movd w/ the other unary ops?

Could add comments partitioning the different instr class clusters.

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp#newcode884
src/IceInstX8632.cpp:884: Str << "mov" <<
TypeX8632Attributes[getDest()->getType()].SdSsString;
This regular mov will overlap a bit w/ the added movss, won't it (yet the movss
doesn't handle movsd)? What is the reason for adding a separate movss?

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2204: #define ALIGN_HACK(Vect)
legalizeToVar(Vect)
Could move ALIGN_HACK down (near the Index < 2), if it's not actually used up
here.

[Jim Stichnoth, 2014-07-17 19:49:02]

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.h#newcode434
src/IceInstX8632.h:434: getDest()->emit(Func);
Hmm, do the existing div/idiv instructions still emit the right asm syntax with
these changes?
[EDIT] oh, now I see the template specialization in the other file, nice.

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:93: default:
Can this be encoded in ICETYPEX8632_TABLE?

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1986: #define ALIGN_HACK(Vect)
legalizeToVar(Vect)
To be absolutely macro-safe, I think this should be legalizeToVar((Vect)).  Here
and below.

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:3227: assert(Slot->getWeight() ==
RegWeight::Zero);
May also want to assert NoRegister.

[wala, 2014-07-17 22:14:12]

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp#newcode477
src/IceInstX8632.cpp:477: template <> const char *InstX8632Lea::Opcode = "lea";
On 2014/07/17 19:36:38, jvoung wrote:
> Cluster lea, and movd w/ the other unary ops?
> 
> Could add comments partitioning the different instr class clusters.

Done.

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp#newcode884
src/IceInstX8632.cpp:884: Str << "mov" <<
TypeX8632Attributes[getDest()->getType()].SdSsString;
On 2014/07/17 19:36:38, jvoung wrote:
> This regular mov will overlap a bit w/ the added movss, won't it (yet the
movss
> doesn't handle movsd)? What is the reason for adding a separate movss?

I need to copy a single floating point value out of a vector into a scalar. This
is normally done with a movss instruction. _mov will not let me do this because
_mov asserts that the width of the destination type needs to be the same as the
width of the source (this is despite the fact that both vectors and scalar
floats are represented in xmm registers).

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:93: default:
On 2014/07/17 19:49:01, stichnot wrote:
> Can this be encoded in ICETYPEX8632_TABLE?

Done.

I've changed the name of this function to getInVectorElementType in the latest
commit, because I think that's a clearer name.

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1986: #define ALIGN_HACK(Vect)
legalizeToVar(Vect)
On 2014/07/17 19:49:01, stichnot wrote:
> To be absolutely macro-safe, I think this should be legalizeToVar((Vect)). 
Here
> and below.

Done.

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:2204: #define ALIGN_HACK(Vect)
legalizeToVar(Vect)
On 2014/07/17 19:36:38, jvoung wrote:
> Could move ALIGN_HACK down (near the Index < 2), if it's not actually used up
> here.

Done.

https://codereview.chromium.org/401523003/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:3227: assert(Slot->getWeight() ==
RegWeight::Zero);
On 2014/07/17 19:49:01, stichnot wrote:
> May also want to assert NoRegister.

Done.

[Jim Stichnoth, 2014-07-18 17:21:01]

Otherwise lgtm.

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp#newcode884
src/IceInstX8632.cpp:884: Str << "mov" <<
TypeX8632Attributes[getDest()->getType()].SdSsString;
On 2014/07/17 22:14:12, wala wrote:
> On 2014/07/17 19:36:38, jvoung wrote:
> > This regular mov will overlap a bit w/ the added movss, won't it (yet the
> movss
> > doesn't handle movsd)? What is the reason for adding a separate movss?
> 
> I need to copy a single floating point value out of a vector into a scalar.
This
> is normally done with a movss instruction. _mov will not let me do this
because
> _mov asserts that the width of the destination type needs to be the same as
the
> width of the source (this is despite the fact that both vectors and scalar
> floats are represented in xmm registers).

OK.  This is something that will need to be cleaned up, though not necessarily
in this CL, so please at least add a TODO here and/or the place where movss is
explicitly used.

https://codereview.chromium.org/401523003/diff/60001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/401523003/diff/60001/src/IceInstX8632.def#new...
src/IceInstX8632.def:86: //#define X(tag, cvt, sdss, width)
Add new elementty macro arg to the comment.

https://codereview.chromium.org/401523003/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:3230: assert(Slot->getWeight() == RegWeight::Zero
&&
Use 2 separate asserts for more precise diagnostics.

[jvoung (off chromium), 2014-07-18 17:56:22]

LGTM modulo Jim's comments

[wala, 2014-07-18 19:36:13]

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/1/src/IceInstX8632.cpp#newcode884
src/IceInstX8632.cpp:884: Str << "mov" <<
TypeX8632Attributes[getDest()->getType()].SdSsString;
On 2014/07/18 17:21:01, stichnot wrote:
> On 2014/07/17 22:14:12, wala wrote:
> > On 2014/07/17 19:36:38, jvoung wrote:
> > > This regular mov will overlap a bit w/ the added movss, won't it (yet the
> > movss
> > > doesn't handle movsd)? What is the reason for adding a separate movss?
> > 
> > I need to copy a single floating point value out of a vector into a scalar.
> This
> > is normally done with a movss instruction. _mov will not let me do this
> because
> > _mov asserts that the width of the destination type needs to be the same as
> the
> > width of the source (this is despite the fact that both vectors and scalar
> > floats are represented in xmm registers).
> 
> OK.  This is something that will need to be cleaned up, though not necessarily
> in this CL, so please at least add a TODO here and/or the place where movss is
> explicitly used.

Done.

https://codereview.chromium.org/401523003/diff/60001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/401523003/diff/60001/src/IceInstX8632.def#new...
src/IceInstX8632.def:86: //#define X(tag, cvt, sdss, width)
On 2014/07/18 17:21:01, stichnot wrote:
> Add new elementty macro arg to the comment.

Done.

https://codereview.chromium.org/401523003/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/401523003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:3230: assert(Slot->getWeight() == RegWeight::Zero
&&
On 2014/07/18 17:21:01, stichnot wrote:
> Use 2 separate asserts for more precise diagnostics.

Done.

[wala, 2014-07-18 19:45:13]

Committed patchset #6 manually as r4988923 (presubmit successful).