[SubZero] Fix f64 to/from i64 moves

[SubZero] Fix f64 to/from i64 moves

The allocation of Hi/Lo part of i64 on stack has been corrected as per MIPS32 ABI. The patch also fixes ZEXT issues occurred while lowering unsigned operations.
Following tests from cross-test framework were testing successfully: (non-vector, ASM mode, Om1, O2)

mem_intrin
TotalTests=114300 Passes=114300 Failures=0

simple_loop
TotalTests=102 Passes=102 Failures=0

test_arith
TotalTests=49489704 Passes=49489704 Failures=0

test_bitmanip
TotalTests=1200 Passes=1200 Failures=0

test_cast
TotalTests=3722 Passes=3722 Failures=0

test_fcmp
TotalTests=123904 Passes=123904 Failures=0

test_global
TotalTests=270 Passes=270 Failures=0

test_icmp
TotalTests=3341520 Passes=3341520 Failures=0

test_strengthreduce
TotalTests=240 Passes=240 Failures=0

Following tests are disabled as they are either all-vectors or contain unimplemented intrinsic lowering:

test_calling_conv
test_select
test_stacksave
test_sync_atomic
test_vector_ops

There are couple of fixes to ARM32 and X86 specific files occurred due to compile-time errors.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-10-19 09:14:54]

Description was changed from

==========
[SubZero] Fix f64 to/from i64 moves

The allocation of Hi/Lo part of i64 on stack has been corrected as per MIPS32
ABI. The patch also fixes ZEXT issues occurred while lowering unsigned
operations.
Following tests from cross-test framework were testing successfully:
(non-vector, ASM mode, Om1, O2)

mem_intrin
TotalTests=114300 Passes=114300 Failures=0

simple_loop
TotalTests=102 Passes=102 Failures=0

test_arith
TotalTests=49489704 Passes=49489704 Failures=0

test_bitmanip
TotalTests=1200 Passes=1200 Failures=0

test_cast
TotalTests=3722 Passes=3722 Failures=0

test_fcmp
TotalTests=123904 Passes=123904 Failures=0

test_global
TotalTests=270 Passes=270 Failures=0

test_icmp
TotalTests=3341520 Passes=3341520 Failures=0

test_strengthreduce
TotalTests=240 Passes=240 Failures=0

Following tests are disabled as they are either all-vectors or contain
unimplemented intrinsic lowering:

test_calling_conv
test_select
test_stacksave
test_sync_atomic
test_vector_ops

There are couple of fixes to ARM32 and X86 specific files occurred due to
compile-time errors.
==========

to

==========
[SubZero] Fix f64 to/from i64 moves

The allocation of Hi/Lo part of i64 on stack has been corrected as per MIPS32
ABI. The patch also fixes ZEXT issues occurred while lowering unsigned
operations.
Following tests from cross-test framework were testing successfully:
(non-vector, ASM mode, Om1, O2)

mem_intrin
TotalTests=114300 Passes=114300 Failures=0

simple_loop
TotalTests=102 Passes=102 Failures=0

test_arith
TotalTests=49489704 Passes=49489704 Failures=0

test_bitmanip
TotalTests=1200 Passes=1200 Failures=0

test_cast
TotalTests=3722 Passes=3722 Failures=0

test_fcmp
TotalTests=123904 Passes=123904 Failures=0

test_global
TotalTests=270 Passes=270 Failures=0

test_icmp
TotalTests=3341520 Passes=3341520 Failures=0

test_strengthreduce
TotalTests=240 Passes=240 Failures=0

Following tests are disabled as they are either all-vectors or contain
unimplemented intrinsic lowering:

test_calling_conv
test_select
test_stacksave
test_sync_atomic
test_vector_ops

There are couple of fixes to ARM32 and X86 specific files occurred due to
compile-time errors.
==========

[jaydeep.patil, 2016-10-19 09:14:54]

jaydeep.patil@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
makdstefan@gmail.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-10-19 18:41:17]

Sorry that the compile warnings/errors slipped through.  I pushed a fix before
this CL was sent out, so please rebase to master...

https://codereview.chromium.org/2432373002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2432373002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:119: // We have sorted the spilled variables
based on their alignment.
Reflow comment to 80-col.  (M-q if using emacs...)

https://codereview.chromium.org/2432373002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:128: for (Variable *Var :
reverse_range(SortedSpilledVariables)) {
This fix concerns me a bit.

The first concern is how this interacts with the register allocator.  On x8632
(and mips32 as well I assume), the hi and lo parts are independently
register-allocated, so either or both parts may end up in registers or on the
stack.  Also in x8632, the lowering is arranged such that bitcasts between i64
and double doesn't rely on the i32 components being adjacent on the stack.  So
it's not clear to me where the hi/lo stack ordering comes into play here.

Maybe the issue is just with respect to function arguments passed on the stack?

The other concern is about the sorting.  SortedSpilledVariables is deliberately
sorted first by alignment (largest to smallest), and in the case of ties,
deterministically sorted by variable number (smallest to largest).  It's
important that it is sorted by largest-to-smallest alignment, since assuming all
alignment values are powers of two, it guarantees that aligning the first one
(with the largest alignment requirement) will also make everything else aligned.
 If you reverse this order, the guarantee no longer holds.

[jaydeep.patil, 2016-10-20 04:35:40]

https://codereview.chromium.org/2432373002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2432373002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:128: for (Variable *Var :
reverse_range(SortedSpilledVariables)) {
On 2016/10/19 18:41:16, Jim Stichnoth wrote:
> This fix concerns me a bit.
> 
> The first concern is how this interacts with the register allocator.  On x8632
> (and mips32 as well I assume), the hi and lo parts are independently
> register-allocated, so either or both parts may end up in registers or on the
> stack.  Also in x8632, the lowering is arranged such that bitcasts between i64
> and double doesn't rely on the i32 components being adjacent on the stack.  So
> it's not clear to me where the hi/lo stack ordering comes into play here.
> 
> Maybe the issue is just with respect to function arguments passed on the
stack?
> 
> The other concern is about the sorting.  SortedSpilledVariables is
deliberately
> sorted first by alignment (largest to smallest), and in the case of ties,
> deterministically sorted by variable number (smallest to largest).  It's
> important that it is sorted by largest-to-smallest alignment, since assuming
all
> alignment values are powers of two, it guarantees that aligning the first one
> (with the largest alignment requirement) will also make everything else
aligned.
>  If you reverse this order, the guarantee no longer holds.

The issue occurs when a spilled i64 is moved to f64. On MIPS we split i64 into
Hi-Lo, but f64 is a single 64-bit variable. To move a spilled i64 to f64 we need
a 64-bit load of f64 from Lo part of i64. However the Lo part is stored at
high-address and its Hi part at low-address which is exactly opposite of the
ABI. 

As mentioned by you, reverse allocation is not a good idea. I should not rely on
the order of the variable on stack.

Let me change it to use registers instead and handle it in post-lower
legalization.

[jaydeep.patil, 2016-10-20 13:21:00]

Addressed review comments.

I have added few MIPS32 specific tests in test_cast cross test to test FP
to/from GP when float/double arguments are passed in GP registers.

[Jim Stichnoth, 2016-10-21 14:18:56]

https://codereview.chromium.org/2432373002/diff/20001/crosstest/test_cast.h
File crosstest/test_cast.h (right):

https://codereview.chromium.org/2432373002/diff/20001/crosstest/test_cast.h#n...
crosstest/test_cast.h:24: #ifdef MIPS32
What do you think about running this set of tests for all targets, not just
MIPS32?  If that were done, I would suggest a few other modifications:

1. Maybe add a dummy int arg both before and after the main arg, in case some
target passes args in the opposite memory order.

2. Would be nice to refactor the great similarities in the .cpp files.  Maybe
use variadic macros?

3. Document somewhere (here?) why these dummy-arg functions are tested
(specifically for bitcast).

https://codereview.chromium.org/2432373002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2432373002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1849: if (DestTy == IceType_f64 && (Offset & 7)
&& IsDstGPReg == false) {
Can you use Utils::IsAligned() for the alignment check?

https://codereview.chromium.org/2432373002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1849: if (DestTy == IceType_f64 && (Offset & 7)
&& IsDstGPReg == false) {
"!IsDstGPReg" instead of "IsDstGPReg == false"

[jaydeep.patil, 2016-10-26 07:47:31]

https://codereview.chromium.org/2432373002/diff/20001/crosstest/test_cast.h
File crosstest/test_cast.h (right):

https://codereview.chromium.org/2432373002/diff/20001/crosstest/test_cast.h#n...
crosstest/test_cast.h:24: #ifdef MIPS32
On 2016/10/21 14:18:56, Jim Stichnoth wrote:
> What do you think about running this set of tests for all targets, not just
> MIPS32?  If that were done, I would suggest a few other modifications:
> 
> 1. Maybe add a dummy int arg both before and after the main arg, in case some
> target passes args in the opposite memory order.
> 
> 2. Would be nice to refactor the great similarities in the .cpp files.  Maybe
> use variadic macros?
> 
> 3. Document somewhere (here?) why these dummy-arg functions are tested
> (specifically for bitcast).

Done.

https://codereview.chromium.org/2432373002/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2432373002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1849: if (DestTy == IceType_f64 && (Offset & 7)
&& IsDstGPReg == false) {
On 2016/10/21 14:18:56, Jim Stichnoth wrote:
> Can you use Utils::IsAligned() for the alignment check?

Done.

https://codereview.chromium.org/2432373002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1849: if (DestTy == IceType_f64 && (Offset & 7)
&& IsDstGPReg == false) {
On 2016/10/21 14:18:56, Jim Stichnoth wrote:
> "!IsDstGPReg" instead of "IsDstGPReg == false"

Done.

[Jim Stichnoth, 2016-10-26 13:15:54]

LGTM.

In the future please also run "make -f Makefile.standalone format".  (I went
ahead and did that.)

[Jim Stichnoth, 2016-10-26 13:16:21]

Description was changed from

==========
[SubZero] Fix f64 to/from i64 moves

The allocation of Hi/Lo part of i64 on stack has been corrected as per MIPS32
ABI. The patch also fixes ZEXT issues occurred while lowering unsigned
operations.
Following tests from cross-test framework were testing successfully:
(non-vector, ASM mode, Om1, O2)

mem_intrin
TotalTests=114300 Passes=114300 Failures=0

simple_loop
TotalTests=102 Passes=102 Failures=0

test_arith
TotalTests=49489704 Passes=49489704 Failures=0

test_bitmanip
TotalTests=1200 Passes=1200 Failures=0

test_cast
TotalTests=3722 Passes=3722 Failures=0

test_fcmp
TotalTests=123904 Passes=123904 Failures=0

test_global
TotalTests=270 Passes=270 Failures=0

test_icmp
TotalTests=3341520 Passes=3341520 Failures=0

test_strengthreduce
TotalTests=240 Passes=240 Failures=0

Following tests are disabled as they are either all-vectors or contain
unimplemented intrinsic lowering:

test_calling_conv
test_select
test_stacksave
test_sync_atomic
test_vector_ops

There are couple of fixes to ARM32 and X86 specific files occurred due to
compile-time errors.
==========

to

==========
[SubZero] Fix f64 to/from i64 moves

The allocation of Hi/Lo part of i64 on stack has been corrected as per MIPS32
ABI. The patch also fixes ZEXT issues occurred while lowering unsigned
operations.
Following tests from cross-test framework were testing successfully:
(non-vector, ASM mode, Om1, O2)

mem_intrin
TotalTests=114300 Passes=114300 Failures=0

simple_loop
TotalTests=102 Passes=102 Failures=0

test_arith
TotalTests=49489704 Passes=49489704 Failures=0

test_bitmanip
TotalTests=1200 Passes=1200 Failures=0

test_cast
TotalTests=3722 Passes=3722 Failures=0

test_fcmp
TotalTests=123904 Passes=123904 Failures=0

test_global
TotalTests=270 Passes=270 Failures=0

test_icmp
TotalTests=3341520 Passes=3341520 Failures=0

test_strengthreduce
TotalTests=240 Passes=240 Failures=0

Following tests are disabled as they are either all-vectors or contain
unimplemented intrinsic lowering:

test_calling_conv
test_select
test_stacksave
test_sync_atomic
test_vector_ops

There are couple of fixes to ARM32 and X86 specific files occurred due to
compile-time errors.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-10-26 13:16:23]

Committed patchset #3 (id:40001) manually as
a7979bfd84bc01c0891169e8d058063fe699cc0e (presubmit successful).

[jaydeep.patil, 2016-10-26 13:17:01]

On 2016/10/26 13:15:54, Jim Stichnoth wrote:
> LGTM.
> 
> In the future please also run "make -f Makefile.standalone format".  (I went
> ahead and did that.)

Thanks Jim.