Subzero. X86. Lowers shufflevector using xmm instructions.

Subzero. X86. Lowers shufflevector using xmm instructions.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4136
R=stichnot@chromium.org

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

[John, 2016-04-25 19:58:34]

jpp@chromium.org changed reviewers:
+ eholk@chromium.org, kschimpf@google.com, sehr@chromium.org,
stichnot@chromium.org

[Jim Stichnoth, 2016-04-25 21:23:45]

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
File crosstest/test_vector_ops.cpp (right):

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
crosstest/test_vector_ops.cpp:25: /* Indexes... */                              
                              \
What does this comment mean?

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
File crosstest/test_vector_ops.h (right):

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
crosstest/test_vector_ops.h:22: // shufflevector, and Subzero_shufflevector, and
shufflevector_count fields.
remove the first "and"

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5573: inline int32_t makePshufdMask(SizeT
Index0, SizeT Index1, SizeT Index2,
I'm wondering whether these SizeT should be explicit uint32_t.

Usually SizeT is meant for values that would normally be size_t, but the PNaCl
bitcode constraints make it safe to limit to 32-bits even when the translator is
built for a 64-bit environment.

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5578: return Mask;
Mask is unsigned, but the function returns a signed value.  Fix this?

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5705: assert(false);
???  Is something got getting tested that ought to be?

https://codereview.chromium.org/1909013002/diff/100001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/vector-shuffle.ll (right):

https://codereview.chromium.org/1909013002/diff/100001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/vector-shuffle.ll:1: ; Some shufflevector optimized
lowering. This list is by no means exhaustive.
reflow to 80-col

[John, 2016-04-25 22:38:39]

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
File crosstest/test_vector_ops.cpp (right):

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
crosstest/test_vector_ops.cpp:25: /* Indexes... */                              
                              \
On 2016/04/25 21:23:44, stichnot wrote:
> What does this comment mean?

variadic macro, i.e., the argument to the X macro:

#define X(...)

I can get rid of Indexes, but

/* ... */

 is even worse than

/* Indexes... */

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
File crosstest/test_vector_ops.h (right):

https://codereview.chromium.org/1909013002/diff/100001/crosstest/test_vector_...
crosstest/test_vector_ops.h:22: // shufflevector, and Subzero_shufflevector, and
shufflevector_count fields.
On 2016/04/25 21:23:45, stichnot wrote:
> remove the first "and"

Done.

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5573: inline int32_t makePshufdMask(SizeT
Index0, SizeT Index1, SizeT Index2,
On 2016/04/25 21:23:45, stichnot wrote:
> I'm wondering whether these SizeT should be explicit uint32_t.
> 
> Usually SizeT is meant for values that would normally be size_t, but the PNaCl
> bitcode constraints make it safe to limit to 32-bits even when the translator
is
> built for a 64-bit environment.

These stem from SizeT values -- i.e., operand indexes. I think this is a
reasonable use for SizeT. Thoughts?

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5578: return Mask;
On 2016/04/25 21:23:45, stichnot wrote:
> Mask is unsigned, but the function returns a signed value.  Fix this?

Mixing signed and unsigned integer types... what could possibly go wrong? Done.

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5705: assert(false);
On 2016/04/25 21:23:45, stichnot wrote:
> ???  Is something got getting tested that ought to be?

Discussed offline.

For posterity: none of the CASE_SRCS_IN(?, ?, ?, 1) will trigger with the
current implementation -- the first element in a shufflevector instructions
always comes from src0.

https://codereview.chromium.org/1909013002/diff/100001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/vector-shuffle.ll (right):

https://codereview.chromium.org/1909013002/diff/100001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/vector-shuffle.ll:1: ; Some shufflevector optimized
lowering. This list is by no means exhaustive.
On 2016/04/25 21:23:45, stichnot wrote:
> reflow to 80-col

Done.

[Jim Stichnoth, 2016-04-25 23:09:03]

otherwise lgtm

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5573: inline int32_t makePshufdMask(SizeT
Index0, SizeT Index1, SizeT Index2,
On 2016/04/25 22:38:39, John wrote:
> On 2016/04/25 21:23:45, stichnot wrote:
> > I'm wondering whether these SizeT should be explicit uint32_t.
> > 
> > Usually SizeT is meant for values that would normally be size_t, but the
PNaCl
> > bitcode constraints make it safe to limit to 32-bits even when the
translator
> is
> > built for a 64-bit environment.
> 
> These stem from SizeT values -- i.e., operand indexes. I think this is a
> reasonable use for SizeT. Thoughts?

I should have added more to my description above of SizeT.  The other aspect is
that if we were to extend PNaCl to be 64-bit, and needed to change SizeT to
uint64_t to allow a function to have a bazillion
variables/instructions/blocks/whatever, we still would not need vector indexes
to be 64-bit.

That said, SizeT is really there so that object fields can be 32-bit instead of
64-bit, and I don't think these indexes are actually stored in objects, so it's
probably a moot point.

https://codereview.chromium.org/1909013002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1909013002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5712: assert(false);
Please change all these to something like:

assert(false && "Following code is untested but likely correct; test and remove
assert.");

[John, 2016-04-26 11:14:13]

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1909013002/diff/100001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5573: inline int32_t makePshufdMask(SizeT
Index0, SizeT Index1, SizeT Index2,
On 2016/04/25 23:09:03, stichnot wrote:
> On 2016/04/25 22:38:39, John wrote:
> > On 2016/04/25 21:23:45, stichnot wrote:
> > > I'm wondering whether these SizeT should be explicit uint32_t.
> > > 
> > > Usually SizeT is meant for values that would normally be size_t, but the
> PNaCl
> > > bitcode constraints make it safe to limit to 32-bits even when the
> translator
> > is
> > > built for a 64-bit environment.
> > 
> > These stem from SizeT values -- i.e., operand indexes. I think this is a
> > reasonable use for SizeT. Thoughts?
> 
> I should have added more to my description above of SizeT.  The other aspect
is
> that if we were to extend PNaCl to be 64-bit, and needed to change SizeT to
> uint64_t to allow a function to have a bazillion
> variables/instructions/blocks/whatever, we still would not need vector indexes
> to be 64-bit.
> 
> That said, SizeT is really there so that object fields can be 32-bit instead
of
> 64-bit, and I don't think these indexes are actually stored in objects, so
it's
> probably a moot point.

Acknowledged.

https://codereview.chromium.org/1909013002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1909013002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5712: assert(false);
On 2016/04/25 23:09:03, stichnot wrote:
> Please change all these to something like:
> 
> assert(false && "Following code is untested but likely correct; test and
remove
> assert.");

Done.

[John, 2016-04-26 11:26:36]

Description was changed from

==========
Subzero. X86. Lowers shufflevector using xmm instructions.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4136
==========

to

==========
Subzero. X86. Lowers shufflevector using xmm instructions.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4136
R=stichnot@chromium.org

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

[John, 2016-04-26 11:26:38]

Committed patchset #8 (id:140001) manually as
ae15f0fd85df9f13a251222d048761a9aa1dcf2a (presubmit successful).