LowerCall for MIPS32

LowerCall for MIPS32

Lower call instructions for MIPS32.

R=stichnot@chromium.org

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

[mohit.bhakkad, 2016-06-13 10:40:32]

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

[mohit.bhakkad, 2016-06-13 10:42:22]

With this patch 64bit.pnacl.ll still fails due to liveliness error, but
uploading it,
as this code might be required by others.

[Jim Stichnoth, 2016-06-13 13:28:58]

On 2016/06/13 10:42:22, mohit.bhakkad wrote:
> With this patch 64bit.pnacl.ll still fails due to liveliness error, but
> uploading it,
> as this code might be required by others.

This patch has conflicts when you try to apply it to latest master.

[mohit.bhakkad, 2016-06-21 08:53:54]

On 2016/06/13 13:28:58, stichnot wrote:
> On 2016/06/13 10:42:22, mohit.bhakkad wrote:
> > With this patch 64bit.pnacl.ll still fails due to liveliness error, but
> > uploading it,
> > as this code might be required by others.
> 
> This patch has conflicts when you try to apply it to latest master.

I have removed the conflicting part, but 64bit.pnacl.ll still fails due to
liveliness error.

[Jim Stichnoth, 2016-06-21 13:22:23]

On 2016/06/21 08:53:54, mohit.bhakkad wrote:
> On 2016/06/13 13:28:58, stichnot wrote:
> > On 2016/06/13 10:42:22, mohit.bhakkad wrote:
> > > With this patch 64bit.pnacl.ll still fails due to liveliness error, but
> > > uploading it,
> > > as this code might be required by others.
> > 
> > This patch has conflicts when you try to apply it to latest master.
> 
> I have removed the conflicting part, but 64bit.pnacl.ll still fails due to
> liveliness error.

I'm still seeing a conflict:

$ git checkout master
Already on 'master'
Your branch is up-to-date with 'origin/master'.
$ git pull
Already up-to-date.
$ git checkout -b mips_lower_call origin/master 
Branch mips_lower_call set up to track remote branch master from origin.
Switched to a new branch 'mips_lower_call'
$ git cl patch 2063653003
<stdin>:160: trailing whitespace.
	
error: patch failed: src/IceTargetLoweringMIPS32.cpp:83
Falling back to three-way merge...
Applied patch to 'src/IceTargetLoweringMIPS32.cpp' with conflicts.
U src/IceTargetLoweringMIPS32.cpp
warning: 1 line adds whitespace errors.
Failed to apply the patch


Actually, I see that "git apply" is not being very smart here, so I was able to
fix it manually and I'm looking at the liveness error.

[Jim Stichnoth, 2016-06-21 13:44:00]

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

https://codereview.chromium.org/2063653003/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:225: assert(Func->validateLiveness());
I recommend moving this assert (and its two comment lines above) to come *after*
the "initial MIPS32 codegen" dump.

Otherwise, it's extremely confusing to try to interpret the liveness validation
errors (running pnacl-sz with -verbose=most or -verbose=all).  This is because
the latest dump was before the instructions were renumbered for liveness
analysis, so the instruction numbers printed in the liveness validation error
messages are "wrong" with respect to the latest dump output.

BTW, if you run manually with a -verbose setting, you'll also want to run with
-threads=0 so you don't get interleaved output/execution of several different
translations.

Finally, I also limit the translation and verbose output to the single
problematic function:
  -translate-only=pass64BitArg
  -verbose-focus=pass64BitArg

https://codereview.chromium.org/2063653003/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1233: Context.insert<InstFakeDef>(ReturnReg);
InstFakeUse

[Jim Stichnoth, 2016-06-21 13:46:17]

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

https://codereview.chromium.org/2063653003/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:225: assert(Func->validateLiveness());
On 2016/06/21 13:44:00, stichnot wrote:
> I recommend moving this assert (and its two comment lines above) to come
*after*
> the "initial MIPS32 codegen" dump.
> 
> Otherwise, it's extremely confusing to try to interpret the liveness
validation
> errors (running pnacl-sz with -verbose=most or -verbose=all).  This is because
> the latest dump was before the instructions were renumbered for liveness
> analysis, so the instruction numbers printed in the liveness validation error
> messages are "wrong" with respect to the latest dump output.
> 
> BTW, if you run manually with a -verbose setting, you'll also want to run with
> -threads=0 so you don't get interleaved output/execution of several different
> translations.
> 
> Finally, I also limit the translation and verbose output to the single
> problematic function:
>   -translate-only=pass64BitArg
>   -verbose-focus=pass64BitArg

BTW, I forgot to mention that this same change ought to be done (by me) in the
ARM and x86 targets, which suffer from the same problem.

[mohit.bhakkad, 2016-06-22 10:07:46]

Thanks Jim for the help, I have moved the assertion location for liveliness in
https://codereview.chromium.org/2080633004, now all tests are passing.

[Jim Stichnoth, 2016-06-22 12:38:31]

lgtm

[Jim Stichnoth, 2016-06-22 12:39:46]

Description was changed from

==========
LowerCall for MIPS32
==========

to

==========
LowerCall for MIPS32

Lower call instructions for MIPS32.
==========

[Jim Stichnoth, 2016-06-22 12:44:10]

Description was changed from

==========
LowerCall for MIPS32

Lower call instructions for MIPS32.
==========

to

==========
LowerCall for MIPS32

Lower call instructions for MIPS32.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-06-22 12:44:12]

Committed patchset #3 (id:40001) manually as
9b4c3c3a41bfb8e9643109d47e8dd26c7f33ba8e (presubmit successful).