[SubZero] Implement address optimization for MIPS

[SubZero] Implement address optimization for MIPS

The patch implements address optimization for MIPS.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-09-07 12:33:36]

Description was changed from

==========
[SubZero] Implement address optimization for MIPS

The patch implements address optimization for MIPS.
==========

to

==========
[SubZero] Implement address optimization for MIPS

The patch implements address optimization for MIPS.
==========

[jaydeep.patil, 2016-09-07 12:33:37]

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

[Jim Stichnoth, 2016-09-09 00:08:22]

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

https://codereview.chromium.org/2313293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2345: const bool IsSingleBB =
(DefNode->getIndex() == UseNode->getIndex());
Why are you adding this single-block condition?

I'm guessing it's because of this in the ARM code:

  // TODO: ensure SrcVar stays single-BB

which was probably just copied over from the original x86 implementation, very
very early in the Subzero design/implementation.

As I recall, the idea was that maybe we should only pull in source variables
from the same basic block, as a heuristic toward controlling register pressure. 
However, in all our analysis of the spec2k benchmarks, we never really
encountered a problem with this, so the TODO was never addressed.

Now, I can't say for sure that there is no register pressure problem from this,
because those kinds of global register allocation effects are hard to discover
just by staring at the code.

So if my guess is correct, I think we should remove this condition for now, to
make it more similar to the other targets, and consider adding it as a separate
CL (perhaps a CL that does this for all targets).

[jaydeep.patil, 2016-09-13 06:42:25]

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

https://codereview.chromium.org/2313293002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2345: const bool IsSingleBB =
(DefNode->getIndex() == UseNode->getIndex());
On 2016/09/09 00:08:22, stichnot wrote:
> Why are you adding this single-block condition?
> 
> I'm guessing it's because of this in the ARM code:
> 
>   // TODO: ensure SrcVar stays single-BB
> 
> which was probably just copied over from the original x86 implementation, very
> very early in the Subzero design/implementation.
> 
> As I recall, the idea was that maybe we should only pull in source variables
> from the same basic block, as a heuristic toward controlling register
pressure. 
> However, in all our analysis of the spec2k benchmarks, we never really
> encountered a problem with this, so the TODO was never addressed.
> 
> Now, I can't say for sure that there is no register pressure problem from
this,
> because those kinds of global register allocation effects are hard to discover
> just by staring at the code.
> 
> So if my guess is correct, I think we should remove this condition for now, to
> make it more similar to the other targets, and consider adding it as a
separate
> CL (perhaps a CL that does this for all targets).  

Done.

[Jim Stichnoth, 2016-09-13 13:09:36]

lgtm

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

Description was changed from

==========
[SubZero] Implement address optimization for MIPS

The patch implements address optimization for MIPS.
==========

to

==========
[SubZero] Implement address optimization for MIPS

The patch implements address optimization for MIPS.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-13 13:09:51]

Committed patchset #2 (id:20001) manually as
47ef0be853cfd485967327aa7f9c67a747a657d8 (presubmit successful).