[Subzero][MIPS32] Implements 64-bit shl, lshr, ashr for MIPS

[Subzero][MIPS32] Implements 64-bit shl, lshr, ashr for MIPS

R=stichnot@chromium.org

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

[sagar.thakur, 2016-09-21 12:35:29]

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

[obucinac, 2016-09-21 12:47:49]

obucinac@gmail.com changed reviewers:
+ obucinac@gmail.com

[obucinac, 2016-09-21 12:47:49]

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1088: using InstMIPS32Movn =
InstMIPS32MovConditional<InstMIPS32::Movn>;
Why is this changed? MovConditional is used by FP movs which are using Floating
Point Condtion Codes for decision. MOVN and MOVZ are using value from GPR for
decision.

[obucinac, 2016-09-21 16:33:31]

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

https://codereview.chromium.org/2359713003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1897: _xor(T3, Src1LoR, T2);
XOR with 0xFFFFFFFF lowers to NOT. For this case, you can use more efficient 
pseudo instruction NOT, based on NOR, available here:

https://codereview.chromium.org/2356293002/

[Jim Stichnoth, 2016-09-21 16:59:25]

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

https://codereview.chromium.org/2359713003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1894: Context.insert<InstFakeUse>(Src1HiR);
Is the FakeUse (here and below) really necessary?  Consider these examples:

define internal i64 @shift1(i64 %opnd, i32 %shift32) {
  %shift64 = zext i32 %shift32 to i64
  br label %next
next:
  %result = shl i64 %opnd, %shift64
  ret i64 %result
}

define internal i64 @shift2(i64 %opnd, i64 %shift1, i64 %shift2) {
  %shift64 = add i64 %shift1, %shift2
  br label %next
next:
  %result = shl i64 %opnd, %shift64
  ret i64 %result
}

(The branch instructions are just so you can more easily see how each
instruction is individually lowered.)

In the first case, the zext puts a zero into the upper half of the i64, and then
the FakeUse forces that unused upper half to stay live.  Without the FakeUse, I
think the zero assignment would be naturally dead-code eliminated.  (This
assumes -O2 compilation).

In the second case, if you don't add the FakeUse, then the 64-bit add involving
the upper halves should be naturally dead-code eliminated.

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right):

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:531: ; MIPS32:
move	[[T1_LO:.*]],[[T_LO]]
trailing whitespace

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:533: ; MIPS32:
movn	[[T_HI]],[[T_LO]],[[T6]]
trailing whitespace

[Jim Stichnoth, 2016-09-21 17:08:21]

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1038: setDestRedefined();
See my comment in
https://codereview.chromium.org/2357143002/diff/1/src/IceTargetLoweringMIPS32.h

[jaydeep.patil, 2016-09-22 09:46:17]

On 2016/09/21 12:47:49, obucinac wrote:
> https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
> File src/IceInstMIPS32.h (right):
> 
>
https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
> src/IceInstMIPS32.h:1088: using InstMIPS32Movn =
> InstMIPS32MovConditional<InstMIPS32::Movn>;
> Why is this changed? MovConditional is used by FP movs which are using
Floating
> Point Condtion Codes for decision. MOVN and MOVZ are using value from GPR for
> decision.

MOVF, MOVT, MOVN and MOVZ are of same type where Dest is both used and defined.
We should handle these types of instructions in MovConditional. 
The third operand could be FCC or a GP reg.

[sagar.thakur, 2016-09-22 14:43:50]

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1038: setDestRedefined();
On 2016/09/21 17:08:20, stichnot wrote:
> See my comment in
>
https://codereview.chromium.org/2357143002/diff/1/src/IceTargetLoweringMIPS32.h

Done.

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

https://codereview.chromium.org/2359713003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1894: Context.insert<InstFakeUse>(Src1HiR);
On 2016/09/21 16:59:25, stichnot wrote:
> Is the FakeUse (here and below) really necessary?  Consider these examples:
> 
> define internal i64 @shift1(i64 %opnd, i32 %shift32) {
>   %shift64 = zext i32 %shift32 to i64
>   br label %next
> next:
>   %result = shl i64 %opnd, %shift64
>   ret i64 %result
> }
> 
> define internal i64 @shift2(i64 %opnd, i64 %shift1, i64 %shift2) {
>   %shift64 = add i64 %shift1, %shift2
>   br label %next
> next:
>   %result = shl i64 %opnd, %shift64
>   ret i64 %result
> }
> 
> (The branch instructions are just so you can more easily see how each
> instruction is individually lowered.)
> 
> In the first case, the zext puts a zero into the upper half of the i64, and
then
> the FakeUse forces that unused upper half to stay live.  Without the FakeUse,
I
> think the zero assignment would be naturally dead-code eliminated.  (This
> assumes -O2 compilation).
> 
> In the second case, if you don't add the FakeUse, then the 64-bit add
involving
> the upper halves should be naturally dead-code eliminated.

Yes, the FakeUse should not be there.

https://codereview.chromium.org/2359713003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1897: _xor(T3, Src1LoR, T2);
On 2016/09/21 16:33:31, obucinac wrote:
> XOR with 0xFFFFFFFF lowers to NOT. For this case, you can use more efficient 
> pseudo instruction NOT, based on NOR, available here:
> 
> https://codereview.chromium.org/2356293002/

Done.

[sagar.thakur, 2016-09-22 14:45:05]

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right):

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:531: ; MIPS32:
move	[[T1_LO:.*]],[[T_LO]]
On 2016/09/21 16:59:25, stichnot wrote:
> trailing whitespace

Done.

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:531: ; MIPS32:
move	[[T1_LO:.*]],[[T_LO]]
On 2016/09/21 16:59:25, stichnot wrote:
> trailing whitespace

Done.

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:533: ; MIPS32:
movn	[[T_HI]],[[T_LO]],[[T6]]
On 2016/09/21 16:59:25, stichnot wrote:
> trailing whitespace

Done.

https://codereview.chromium.org/2359713003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/64bit.pnacl.ll:533: ; MIPS32:
movn	[[T_HI]],[[T_LO]],[[T6]]
On 2016/09/21 16:59:25, stichnot wrote:
> trailing whitespace

Done.

[Jim Stichnoth, 2016-09-23 14:10:37]

I'm seeing some "make check-lit" failures, can you look into those?

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1088: using InstMIPS32Movn =
InstMIPS32MovConditional<InstMIPS32::Movn>;
On 2016/09/21 12:47:49, obucinac wrote:
> Why is this changed? MovConditional is used by FP movs which are using
Floating
> Point Condtion Codes for decision. MOVN and MOVZ are using value from GPR for
> decision.

Is there an answer for this question?  (I'm not familiar enough to weight in.)

[obucinac, 2016-09-23 15:59:45]

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1088: using InstMIPS32Movn =
InstMIPS32MovConditional<InstMIPS32::Movn>;
On 2016/09/23 14:10:37, stichnot wrote:
> On 2016/09/21 12:47:49, obucinac wrote:
> > Why is this changed? MovConditional is used by FP movs which are using
> Floating
> > Point Condtion Codes for decision. MOVN and MOVZ are using value from GPR
for
> > decision.
> 
> Is there an answer for this question?  (I'm not familiar enough to weight in.)

The main difference  is that MovConditional has no Dest, and ThreeAddrGPR has. I
am concerned that this change may affect propper liveness analisys.

[Jim Stichnoth, 2016-09-23 18:48:51]

On 2016/09/23 14:10:37, stichnot wrote:
> I'm seeing some "make check-lit" failures, can you look into those?

Never mind - I think this came from "git cl patch" doing an automatic merge
incorrectly.

[Jim Stichnoth, 2016-09-23 18:49:11]

lgtm

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/40001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1088: using InstMIPS32Movn =
InstMIPS32MovConditional<InstMIPS32::Movn>;
On 2016/09/23 15:59:45, obucinac wrote:
> On 2016/09/23 14:10:37, stichnot wrote:
> > On 2016/09/21 12:47:49, obucinac wrote:
> > > Why is this changed? MovConditional is used by FP movs which are using
> > Floating
> > > Point Condtion Codes for decision. MOVN and MOVZ are using value from GPR
> for
> > > decision.
> > 
> > Is there an answer for this question?  (I'm not familiar enough to weight
in.)
> 
> The main difference  is that MovConditional has no Dest, and ThreeAddrGPR has.
I
> am concerned that this change may affect propper liveness analisys.

I've looked through this more carefully.  I think the original code is wrong and
the proposed change is correct.

If I understand correctly:
  movn A, B, C
sets A to either A or B, based on the value of C.  In this case, it is correct
to have A as both the dest and one of the src operands, and this is what
InstMIPS32MovConditional does.

On the other hand, if the instruction were to set A based on just B and C, and
completely ignored the old value in A, then ThreeAddrGPR would be more
appropriate.

https://codereview.chromium.org/2359713003/diff/100001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2359713003/diff/100001/src/IceInstMIPS32.h#ne...
src/IceInstMIPS32.h:1038: addSource(Dest);
I think this change should be reverted - it should already be fixed (by
https://codereview.chromium.org/2357143002) after rebasing to master.

[I went ahead and fixed it manually.]

[Jim Stichnoth, 2016-09-23 18:55:48]

Description was changed from

==========
[Subzero][MIPS32] Implements 64-bit shl, lshr, ashr for MIPS
==========

to

==========
[Subzero][MIPS32] Implements 64-bit shl, lshr, ashr for MIPS

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-23 18:55:51]

Committed patchset #6 (id:100001) manually as
86b60ef8a40d2c19f4595d065f3de5b8ea0a0e9d (presubmit successful).