Lower casting operations that involve vector types.

Lower casting operations that involve vector types.

Impacted instructions:

bitcast {v4f32, v4i32, v8i16, v16i8} <-> {v4f32, v4i32, v8i16, v16i8}
bitcast v8i1 <-> i8
bitcast v16i1 <-> i16

(There was already code present to handle trivial bitcasts like v16i1 <-> v16i1.)

[sz]ext v4i1 -> v4i32
[sz]ext v8i1 -> v8i16
[sz]ext v16i1 -> v16i8

trunc v4i32 -> v4i1
trunc v8i16 -> v8i1
trunc v16i8 -> v16i1

[su]itofp v4i32 -> v4f32
fpto[su]i v4f32 -> v4i32

Where there is a relatively simple lowering to x86 instructions, it has been used. Otherwise a helper call is used.

Some lowerings require a materialization of a integer vector with 1s in each entry. Since there is no support for vector constant pools, the constant is materialized purely through register operations.

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=83b8036

[Jim Stichnoth, 2014-07-14 18:33:12]

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

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1527: Variable *OneMask = makeReg(DestTy);
Seems to be substantial code duplication across the 3 sections that use
OneMask/MinusOne. Consider refactoring them into a private helper method?

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1544: SizeT ShiftAmount = 8 *
typeWidthInBytes(typeElementType(DestTy)) - 1;
Use CHAR_BIT instead of 8

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1648: _cvt(T, Src0RM);
Looks plausible, I guess, but I'm looking forward to the cross tests on all the
corner cases, especially for conversions between integer and FP. :)

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1681: InstCall *Call =
makeHelperCall("__fptoui_v4f32", Dest, MaxSrcs);
Here and elsewhere, the helper function will be part of a Subzero compiler-rt,
so avoid defining functions whose names are a reserved identifier pattern, such
as starting with an underscore or containing a sequence of two underscores. 
(Search for "C++ reserved identifiers".)

[wala, 2014-07-14 20:52:23]

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

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1648: _cvt(T, Src0RM);
On 2014/07/14 18:33:12, stichnot wrote:
> Looks plausible, I guess, but I'm looking forward to the cross tests on all
the
> corner cases, especially for conversions between integer and FP. :)

This is what LLVM does.

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1681: InstCall *Call =
makeHelperCall("__fptoui_v4f32", Dest, MaxSrcs);
On 2014/07/14 18:33:12, stichnot wrote:
> Here and elsewhere, the helper function will be part of a Subzero compiler-rt,
> so avoid defining functions whose names are a reserved identifier pattern,
such
> as starting with an underscore or containing a sequence of two underscores. 
> (Search for "C++ reserved identifiers".)

Such identifiers are reserved to the implementation. Isn't Subzero going to be
the implementation?

[Jim Stichnoth, 2014-07-14 22:23:31]

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

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1681: InstCall *Call =
makeHelperCall("__fptoui_v4f32", Dest, MaxSrcs);
On 2014/07/14 20:52:22, wala wrote:
> On 2014/07/14 18:33:12, stichnot wrote:
> > Here and elsewhere, the helper function will be part of a Subzero
compiler-rt,
> > so avoid defining functions whose names are a reserved identifier pattern,
> such
> > as starting with an underscore or containing a sequence of two underscores. 
> > (Search for "C++ reserved identifiers".)
> 
> Such identifiers are reserved to the implementation. Isn't Subzero going to be
> the implementation?

I wouldn't say Subzero will be the *complete* implementation.  For example,
these helpers are probably compiled and translated by the PNaCl/LLVM toolchain,
and there's also the IRT interface.  I think it's safest to stay out of the way
of conflicts at that level (as unlikely as they may be).

At some point we'll have to go over the set of helpers, clean them up, make
naming consistent, etc...

[jvoung (off chromium), 2014-07-15 00:24:11]

https://codereview.chromium.org/383303003/diff/60001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-cast.ll (right):

https://codereview.chromium.org/383303003/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-cast.ll:19: ; CHECK-NEXT: ret
Are these the only cases where OPTM1 differs from CHECK?

Just wondering if it could be simplified, by splitting this test, to check the
same thing in both opt levels.

[wala, 2014-07-15 00:41:10]

https://codereview.chromium.org/383303003/diff/60001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-cast.ll (right):

https://codereview.chromium.org/383303003/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-cast.ll:19: ; CHECK-NEXT: ret
On 2014/07/15 00:24:11, jvoung wrote:
> Are these the only cases where OPTM1 differs from CHECK?
> 
> Just wondering if it could be simplified, by splitting this test, to check the
> same thing in both opt levels.

Yes, the test for bitcast is basically checking that bitcast is lowered to a
no-op in -O2, because -O2 should be smart enough to do this. There is some
additional register / memory shuffling code that makes Om1 output less easy to
test.

I can move the bitcast tests out to a separate file.

[wala, 2014-07-15 22:52:22]

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

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1527: Variable *OneMask = makeReg(DestTy);
On 2014/07/14 18:33:12, stichnot wrote:
> Seems to be substantial code duplication across the 3 sections that use
> OneMask/MinusOne. Consider refactoring them into a private helper method?

Done.

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1544: SizeT ShiftAmount = 8 *
typeWidthInBytes(typeElementType(DestTy)) - 1;
On 2014/07/14 18:33:12, stichnot wrote:
> Use CHAR_BIT instead of 8

Since CHAR_BIT depends on the machine the code is running on, I've added a new
constant X86_CHAR_BIT instead.

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1681: InstCall *Call =
makeHelperCall("__fptoui_v4f32", Dest, MaxSrcs);
On 2014/07/14 22:23:30, stichnot wrote:
> On 2014/07/14 20:52:22, wala wrote:
> > On 2014/07/14 18:33:12, stichnot wrote:
> > > Here and elsewhere, the helper function will be part of a Subzero
> compiler-rt,
> > > so avoid defining functions whose names are a reserved identifier pattern,
> > such
> > > as starting with an underscore or containing a sequence of two
underscores. 
> > > (Search for "C++ reserved identifiers".)
> > 
> > Such identifiers are reserved to the implementation. Isn't Subzero going to
be
> > the implementation?
> 
> I wouldn't say Subzero will be the *complete* implementation.  For example,
> these helpers are probably compiled and translated by the PNaCl/LLVM
toolchain,
> and there's also the IRT interface.  I think it's safest to stay out of the
way
> of conflicts at that level (as unlikely as they may be).
> 
> At some point we'll have to go over the set of helpers, clean them up, make
> naming consistent, etc...

Done.

All helpers have a Sz_ prefix.

https://codereview.chromium.org/383303003/diff/60001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-cast.ll (right):

https://codereview.chromium.org/383303003/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-cast.ll:19: ; CHECK-NEXT: ret
On 2014/07/15 00:41:09, wala wrote:
> On 2014/07/15 00:24:11, jvoung wrote:
> > Are these the only cases where OPTM1 differs from CHECK?
> > 
> > Just wondering if it could be simplified, by splitting this test, to check
the
> > same thing in both opt levels.
> 
> Yes, the test for bitcast is basically checking that bitcast is lowered to a
> no-op in -O2, because -O2 should be smart enough to do this. There is some
> additional register / memory shuffling code that makes Om1 output less easy to
> test.
> 
> I can move the bitcast tests out to a separate file.

Done.

[Jim Stichnoth, 2014-07-15 23:15:44]

lgtm

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

https://codereview.chromium.org/383303003/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1544: SizeT ShiftAmount = 8 *
typeWidthInBytes(typeElementType(DestTy)) - 1;
On 2014/07/15 22:52:22, wala wrote:
> On 2014/07/14 18:33:12, stichnot wrote:
> > Use CHAR_BIT instead of 8
> 
> Since CHAR_BIT depends on the machine the code is running on, I've added a new
> constant X86_CHAR_BIT instead.

Good point, thanks.

[jvoung (off chromium), 2014-07-16 00:01:38]

LGTM

https://codereview.chromium.org/383303003/diff/110001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/vector-bitcast.ll (right):

https://codereview.chromium.org/383303003/diff/110001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-bitcast.ll:189: ; CHECK: call
Sz_bitcast_v16i1_to_i16
Could add the OPTM1 check here too?

https://codereview.chromium.org/383303003/diff/110001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-bitcast.ll:192: define <8 x i1>
@test_bitcast_i8_to_v8i1(i8 %arg) {
Technically, the PNaCl bitcode ABI doesn't allow arguments and return values to
be smaller than i32.

This mostly causes a problem on a function call (push), but it might be good to
make this test conform to that ABI.

[wala, 2014-07-16 17:19:02]

https://codereview.chromium.org/383303003/diff/110001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/vector-bitcast.ll (right):

https://codereview.chromium.org/383303003/diff/110001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-bitcast.ll:189: ; CHECK: call
Sz_bitcast_v16i1_to_i16
On 2014/07/16 00:01:38, jvoung wrote:
> Could add the OPTM1 check here too?

Done.

https://codereview.chromium.org/383303003/diff/110001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-bitcast.ll:192: define <8 x i1>
@test_bitcast_i8_to_v8i1(i8 %arg) {
On 2014/07/16 00:01:38, jvoung wrote:
> Technically, the PNaCl bitcode ABI doesn't allow arguments and return values
to
> be smaller than i32.
> 
> This mostly causes a problem on a function call (push), but it might be good
to
> make this test conform to that ABI.

Done.

[wala, 2014-07-16 17:21:33]

Committed patchset #7 manually as r83b8036 (presubmit successful).