[SubZero] Implement lowerSwitch for MIPS

[SubZero] Implement lowerSwitch for MIPS

The patch implements lowerSwitch for i32 and i64 types.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-08-30 03:40:07]

Description was changed from

==========
[SubZero] Implement lowerSwitch for MIPS

The patch implements lowerSwitch for i32 and i64 types.
==========

to

==========
[SubZero] Implement lowerSwitch for MIPS

The patch implements lowerSwitch for i32 and i64 types.
==========

[jaydeep.patil, 2016-08-30 03:40:07]

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

[Jim Stichnoth, 2016-08-30 13:20:36]

https://codereview.chromium.org/2289043002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2289043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:482: getSrc(0)->emit(Func);
These two emit() calls should be dump().

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

https://codereview.chromium.org/2289043002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2332: ValueHi = legalize(ValueHi, Legal_Reg);
Just use legalizeToReg() here for for ValueLo?

https://codereview.chromium.org/2289043002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2348: CfgNode *NoTargetFlase = nullptr;
s/Flase/False/

also, use "static constexpr" or "constexpr" for these self-documenting constants
(Subzero isn't consistent about whether to use "static")

https://codereview.chromium.org/2289043002/diff/1/tests_lit/llvm2ice_tests/sw...
File tests_lit/llvm2ice_tests/switch-opt.ll (right):

https://codereview.chromium.org/2289043002/diff/1/tests_lit/llvm2ice_tests/sw...
tests_lit/llvm2ice_tests/switch-opt.ll:55: ; MIPS32: beq	{{.*}},{{.*}},{{.*}}
<.LtestSwitch$sw.epilog>
Two problems with checking label strings:

1. For filetype=asm and filetype=iasm, we don't really guarantee that label
names are stable over time.  That is, in the future we might have some reason to
change something about the way names are generated, and then all these tests
would spuriously break.

2. For filetype=obj, there are no label strings in the disassembled output, just
code addresses.

I think it would be more robust to use this pattern:
http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables
e.g.:

MIPS32: li ...,1
MIPS32: beq ... <[[SW_EPILOG:.*]]>
MIPS32: li ...,2
MIPS32: beq ... <[[SW_EPILOG]]>
MIPS32: li ...,3
MIPS32: beq ... <[[SW_EPILOG]]>
...
MIPS32: [[SW_EPILOG]]
MIPS32: jr ra

And then similar for the other labels.

[jaydeep.patil, 2016-08-31 04:10:23]

https://codereview.chromium.org/2289043002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2289043002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:482: getSrc(0)->emit(Func);
On 2016/08/30 13:20:36, stichnot wrote:
> These two emit() calls should be dump().

Done.

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

https://codereview.chromium.org/2289043002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2332: ValueHi = legalize(ValueHi, Legal_Reg);
On 2016/08/30 13:20:36, stichnot wrote:
> Just use legalizeToReg() here for for ValueLo?

Done.

https://codereview.chromium.org/2289043002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2348: CfgNode *NoTargetFlase = nullptr;
On 2016/08/30 13:20:36, stichnot wrote:
> s/Flase/False/
> 
> also, use "static constexpr" or "constexpr" for these self-documenting
constants
> (Subzero isn't consistent about whether to use "static")

Done.

https://codereview.chromium.org/2289043002/diff/1/tests_lit/llvm2ice_tests/sw...
File tests_lit/llvm2ice_tests/switch-opt.ll (right):

https://codereview.chromium.org/2289043002/diff/1/tests_lit/llvm2ice_tests/sw...
tests_lit/llvm2ice_tests/switch-opt.ll:55: ; MIPS32: beq	{{.*}},{{.*}},{{.*}}
<.LtestSwitch$sw.epilog>
On 2016/08/30 13:20:36, stichnot wrote:
> Two problems with checking label strings:
> 
> 1. For filetype=asm and filetype=iasm, we don't really guarantee that label
> names are stable over time.  That is, in the future we might have some reason
to
> change something about the way names are generated, and then all these tests
> would spuriously break.
> 
> 2. For filetype=obj, there are no label strings in the disassembled output,
just
> code addresses.
> 
> I think it would be more robust to use this pattern:
> http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables
> e.g.:
> 
> MIPS32: li ...,1
> MIPS32: beq ... <[[SW_EPILOG:.*]]>
> MIPS32: li ...,2
> MIPS32: beq ... <[[SW_EPILOG]]>
> MIPS32: li ...,3
> MIPS32: beq ... <[[SW_EPILOG]]>
> ...
> MIPS32: [[SW_EPILOG]]
> MIPS32: jr ra
> 
> And then similar for the other labels.

Done.

[Jim Stichnoth, 2016-08-31 05:38:36]

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

https://codereview.chromium.org/2289043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2348: constexpr CfgNode *NoTargetFlase =
nullptr;
s/Flase/False/

[jaydeep.patil, 2016-08-31 05:43:43]

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

https://codereview.chromium.org/2289043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2348: constexpr CfgNode *NoTargetFlase =
nullptr;
On 2016/08/31 05:38:35, stichnot wrote:
> s/Flase/False/

Done.

[Jim Stichnoth, 2016-08-31 12:09:51]

lgtm

[Jim Stichnoth, 2016-08-31 12:10:07]

Description was changed from

==========
[SubZero] Implement lowerSwitch for MIPS

The patch implements lowerSwitch for i32 and i64 types.
==========

to

==========
[SubZero] Implement lowerSwitch for MIPS

The patch implements lowerSwitch for i32 and i64 types.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-08-31 12:10:08]

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