[SubZero] Handle relocatable constants for MIPS

[SubZero] Handle relocatable constants for MIPS

The patch generates HI/LO modifiers for relocatable constants.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-10-14 07:52:49]

Description was changed from

==========
[SubZero] Handle relocatable constants for MIPS

The patch generates HI/LO modifiers for relocatable constants.
==========

to

==========
[SubZero] Handle relocatable constants for MIPS

The patch generates HI/LO modifiers for relocatable constants.
==========

[jaydeep.patil, 2016-10-14 07:52:49]

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-16 14:44:43]

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

https://codereview.chromium.org/2420033002/diff/1/src/IceInstMIPS32.h#newcode...
src/IceInstMIPS32.h:1014: if (Reloc != RO_No) {
All other things equal, I think this style of code:

  if (C) A; else B;

is easier to read than:

  if (!C) B; else A;

So I would either use "if (Reloc == RO_No)" and swap the if/else blocks, or
something like "if (isReloc(Reloc))" - though I wish I could think of a better
name than isReloc - and provide an inline bool definition of isReloc() near the
top of the file.

Same comment below.

https://codereview.chromium.org/2420033002/diff/1/src/IceInstMIPS32.h#newcode...
src/IceInstMIPS32.h:1078: const RelocOp Reloc = RO_No;
I think there should not be an initializer here, since all ctors explicitly
initialize Reloc based on a ctor arg.

(And then if a new ctor is ever added, and it forgets to provide an
initialization of Reloc, the compiler should warn.)

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

https://codereview.chromium.org/2420033002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4850: // TODO(jaydeep.patil): Handle ST_Nonsfi
case.
See my comment in
https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone#newcode413
.

In short, nonsfi doesn't need support in MIPS.  It was added experimentally in
x86 to support another group here, but that group has moved on from Subzero, so
MIPS really only needs the native and sandboxed models.

[jaydeep.patil, 2016-10-17 04:05:02]

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

https://codereview.chromium.org/2420033002/diff/1/src/IceInstMIPS32.h#newcode...
src/IceInstMIPS32.h:1014: if (Reloc != RO_No) {
On 2016/10/16 14:44:43, Jim Stichnoth wrote:
> All other things equal, I think this style of code:
> 
>   if (C) A; else B;
> 
> is easier to read than:
> 
>   if (!C) B; else A;
> 
> So I would either use "if (Reloc == RO_No)" and swap the if/else blocks, or
> something like "if (isReloc(Reloc))" - though I wish I could think of a better
> name than isReloc - and provide an inline bool definition of isReloc() near
the
> top of the file.
> 
> Same comment below.

Done.

https://codereview.chromium.org/2420033002/diff/1/src/IceInstMIPS32.h#newcode...
src/IceInstMIPS32.h:1078: const RelocOp Reloc = RO_No;
On 2016/10/16 14:44:43, Jim Stichnoth wrote:
> I think there should not be an initializer here, since all ctors explicitly
> initialize Reloc based on a ctor arg.
> 
> (And then if a new ctor is ever added, and it forgets to provide an
> initialization of Reloc, the compiler should warn.)

Done.

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

https://codereview.chromium.org/2420033002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4850: // TODO(jaydeep.patil): Handle ST_Nonsfi
case.
On 2016/10/16 14:44:43, Jim Stichnoth wrote:
> See my comment in
>
https://codereview.chromium.org/2085303002/diff/1/Makefile.standalone#newcode413
> .
> 
> In short, nonsfi doesn't need support in MIPS.  It was added experimentally in
> x86 to support another group here, but that group has moved on from Subzero,
so
> MIPS really only needs the native and sandboxed models.

Done.

[Jim Stichnoth, 2016-10-17 04:50:24]

lgtm

[Jim Stichnoth, 2016-10-17 04:50:45]

Description was changed from

==========
[SubZero] Handle relocatable constants for MIPS

The patch generates HI/LO modifiers for relocatable constants.
==========

to

==========
[SubZero] Handle relocatable constants for MIPS

The patch generates HI/LO modifiers for relocatable constants.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-10-17 04:50:47]

Committed patchset #2 (id:20001) manually as
45e4d5edca13d208bb65cb0423264ab9d9c2f08a (presubmit successful).