Subzero, MIPS32: lowerUnreachable

Subzero, MIPS32: lowerUnreachable

Patch implements lowerUnreachable and encoding for teq
instruction. To avoid duplicated code, class describing
trap instruction is borrowed from
https://codereview.chromium.org/2339323004/

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

[obucinac, 2016-09-19 18:51:03]

Description was changed from

==========
Subzero, MIPS32: lowerUnreachable

Patch implements lowerUnreachable and encoding for teq
instruction. To avoid duplicated code, class describing
trap instruction is borrowed from
https://codereview.chromium.org/2339323004/
==========

to

==========
Subzero, MIPS32: lowerUnreachable

Patch implements lowerUnreachable and encoding for teq
instruction. To avoid duplicated code, class describing
trap instruction is borrowed from
https://codereview.chromium.org/2339323004/
==========

[obucinac, 2016-09-19 18:51:03]

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

[Jim Stichnoth, 2016-09-19 23:56:38]

Also, could you rebase to latest master?

https://codereview.chromium.org/2350903002/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2350903002/diff/1/src/IceInstMIPS32.h#newcode918
src/IceInstMIPS32.h:918: template <InstMIPS32::InstKindMIPS32 K>
I guess I should have asked this in the previous review.

Why does the Trap instruction need to be templated?  Will it ever take an
InstKind besides Teq?  Or could it be structured more like non-templated
InstMIPS32Ret?

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

https://codereview.chromium.org/2350903002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3362: void TargetMIPS32::lowerUnreachable(const
InstUnreachable * /*Instr*/) {
I would probably just remove the Instr name:

void TargetMIPS32::lowerUnreachable(const InstUnreachable *) {

https://codereview.chromium.org/2350903002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3363: _teq(getZero(), getZero(), 0);
Can you "document" the 0?

constexpr uint32_t TrapCodeForWhatever = 0;

[Jim Stichnoth, 2016-09-20 00:02:08]

It's possible that I rebased wrong, but I'm getting an error on "make -f
Makefile.standalone check-lit".  Relevant piece of the output:

Command 9 Stderr:
/ssd/stichnot/nacltest/native_client/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/unreachable.ll:63:11:
error: expected string not found in input
; MIPS32: jal {{.*}} __divsi3
          ^
<stdin>:25:1: note: scanning from here
00000038 <.Ldivide$return>:

[obucinac, 2016-09-20 00:14:31]

On 2016/09/20 00:02:08, stichnot wrote:
> It's possible that I rebased wrong, but I'm getting an error on "make -f
> Makefile.standalone check-lit".  Relevant piece of the output:
> 
> Command 9 Stderr:
>
/ssd/stichnot/nacltest/native_client/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/unreachable.ll:63:11:
> error: expected string not found in input
> ; MIPS32: jal {{.*}} __divsi3
>           ^
> <stdin>:25:1: note: scanning from here
> 00000038 <.Ldivide$return>:

No, its fine, because previous patch, from where I copied class describing trap
instruction, replaces call to __divsi3 with hardware instruction.

Regarding templated trap class, I can change it, but You just merged it into the
source code with patch https://codereview.chromium.org/2339323004/

Will be fixed by tomorrow.

[jaydeep.patil, 2016-09-20 03:16:54]

https://codereview.chromium.org/2350903002/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2350903002/diff/1/src/IceInstMIPS32.h#newcode918
src/IceInstMIPS32.h:918: template <InstMIPS32::InstKindMIPS32 K>
On 2016/09/19 23:56:38, stichnot wrote:
> I guess I should have asked this in the previous review.
> 
> Why does the Trap instruction need to be templated?  Will it ever take an
> InstKind besides Teq?  Or could it be structured more like non-templated
> InstMIPS32Ret?

There 11 more trap instructions (like TNE, TGE, TLT etc.).

[obucinac, 2016-09-20 13:06:02]

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

https://codereview.chromium.org/2350903002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3362: void TargetMIPS32::lowerUnreachable(const
InstUnreachable * /*Instr*/) {
On 2016/09/19 23:56:38, stichnot wrote:
> I would probably just remove the Instr name:
> 
> void TargetMIPS32::lowerUnreachable(const InstUnreachable *) {

Done.

https://codereview.chromium.org/2350903002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3363: _teq(getZero(), getZero(), 0);
On 2016/09/19 23:56:38, stichnot wrote:
> Can you "document" the 0?
> 
> constexpr uint32_t TrapCodeForWhatever = 0;

Done.

[Jim Stichnoth, 2016-09-20 13:49:55]

Description was changed from

==========
Subzero, MIPS32: lowerUnreachable

Patch implements lowerUnreachable and encoding for teq
instruction. To avoid duplicated code, class describing
trap instruction is borrowed from
https://codereview.chromium.org/2339323004/
==========

to

==========
Subzero, MIPS32: lowerUnreachable

Patch implements lowerUnreachable and encoding for teq
instruction. To avoid duplicated code, class describing
trap instruction is borrowed from
https://codereview.chromium.org/2339323004/

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

[Jim Stichnoth, 2016-09-20 13:49:57]

Committed patchset #2 (id:20001) manually as
3b61d70e2f05fa692ee759c9233dc6b8f341a2fc (presubmit successful).

[Jim Stichnoth, 2016-09-20 13:50:06]

lgtm