[SubZero] Implement load and store for MIPS

[SubZero] Implement load and store for MIPS

This patch implements lowerLoad and extends existing lowerStore for byte, short and floating-point types.
The patch also modifies PostLoweringLegalizer for conversion of mov to load or store.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-09-02 11:39:23]

Description was changed from

==========
[SubZero] Implement load and store for MIPS

This patch implements lowerLoad and extends existing lowerStore for byte, short
and floating-point types.
The patch also modifies PostLoweringLegalizer for conversion of mov to load or
store.
==========

to

==========
[SubZero] Implement load and store for MIPS

This patch implements lowerLoad and extends existing lowerStore for byte, short
and floating-point types.
The patch also modifies PostLoweringLegalizer for conversion of mov to load or
store.
==========

[jaydeep.patil, 2016-09-02 11:39:23]

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-09-03 13:47:13]

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

https://codereview.chromium.org/2301303003/diff/1/src/IceInstMIPS32.h#newcode532
src/IceInstMIPS32.h:532: if (Ty == IceType_i8 || Ty == IceType_i1) {
Maybe use a switch statement instead?  (here and below)

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

https://codereview.chromium.org/2301303003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1096: // FP arguments are passed in GP reg if
first argument is
"is is in GP" ==> "is in GP"

Also, maybe reflow comment to 80-col.

[jaydeep.patil, 2016-09-04 06:24:30]

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

https://codereview.chromium.org/2301303003/diff/1/src/IceInstMIPS32.h#newcode532
src/IceInstMIPS32.h:532: if (Ty == IceType_i8 || Ty == IceType_i1) {
On 2016/09/03 13:47:13, stichnot wrote:
> Maybe use a switch statement instead?  (here and below)

Done.

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

https://codereview.chromium.org/2301303003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1096: // FP arguments are passed in GP reg if
first argument is
On 2016/09/03 13:47:13, stichnot wrote:
> "is is in GP" ==> "is in GP"
> 
> Also, maybe reflow comment to 80-col.

Done.

[Jim Stichnoth, 2016-09-04 14:19:00]

lgtm

https://codereview.chromium.org/2301303003/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2301303003/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:535: Str << "\t"
A few suggestions/possibilities for future CLs.

1. Instead of this:
  Str << "\t" << "xxx" << "\t"
you can do this:
  Str << "\t" "xxx" "\t"
which preserves the ability to find such text via "git grep -w xxx" but also
results in a smaller, faster pnacl-sz binary.  We try to use this pattern in
general, but I'm sure there are other places where this has slipped through.

2. You could simplify this by moving the two instances of "\t" outside the
switch statement.

3. This is a pattern where a string is modified according to some Type value. 
In Subzero, this is usually handled through static tables defined by x-macros. 
See for example the various string constants in ICETYPEARM32_TABLE or
ICETYPEX8664_TABLE.

[Jim Stichnoth, 2016-09-04 14:19:13]

Description was changed from

==========
[SubZero] Implement load and store for MIPS

This patch implements lowerLoad and extends existing lowerStore for byte, short
and floating-point types.
The patch also modifies PostLoweringLegalizer for conversion of mov to load or
store.
==========

to

==========
[SubZero] Implement load and store for MIPS

This patch implements lowerLoad and extends existing lowerStore for byte, short
and floating-point types.
The patch also modifies PostLoweringLegalizer for conversion of mov to load or
store.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-04 14:19:14]

Committed patchset #2 (id:20001) manually as
1d0690bc3c62c025a43e5f7cb2f3a916ec48e556 (presubmit successful).

[jaydeep.patil, 2016-09-05 04:51:48]

Thanks for the review.

https://codereview.chromium.org/2301303003/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2301303003/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:535: Str << "\t"
On 2016/09/04 14:19:00, stichnot wrote:
> A few suggestions/possibilities for future CLs.
> 
> 1. Instead of this:
>   Str << "\t" << "xxx" << "\t"
> you can do this:
>   Str << "\t" "xxx" "\t"
> which preserves the ability to find such text via "git grep -w xxx" but also
> results in a smaller, faster pnacl-sz binary.  We try to use this pattern in
> general, but I'm sure there are other places where this has slipped through.
> 
> 2. You could simplify this by moving the two instances of "\t" outside the
> switch statement.
> 
> 3. This is a pattern where a string is modified according to some Type value. 
> In Subzero, this is usually handled through static tables defined by x-macros.

> See for example the various string constants in ICETYPEARM32_TABLE or
> ICETYPEX8664_TABLE.

Acknowledged.