Subzero, MIPS32: Changes for improving sandbox crosstest results

Subzero, MIPS32: Changes for improving sandbox crosstest results

R=stichnot@chromium.org

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

[Stefan Maksimovic, 2016-11-21 09:48:37]

makdstefan@gmail.com changed reviewers:
+ jaydeep.patil@imgtec.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-11-21 20:08:39]

https://codereview.chromium.org/2519863002/diff/1/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2519863002/diff/1/pydir/crosstest.py#newcode161
pydir/crosstest.py:161: '-triple=' + ('mipsel-linux-gnu'
Is it possible to use a nacl-specific tuple for MIPS?  This is what the other
targets do, and that may also mean that the new .bundle_align_mode directive in
IceTargetLoweringMIPS32.cpp would not be needed.

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

https://codereview.chromium.org/2519863002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:5847: OstreamLocker L(Ctx);
Just noticed this...  Use the normal pattern at the start of this method:

  if (!BuildDefs::dump())
    return;

[Stefan Maksimovic, 2016-11-22 11:38:13]

https://codereview.chromium.org/2519863002/diff/1/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2519863002/diff/1/pydir/crosstest.py#newcode161
pydir/crosstest.py:161: '-triple=' + ('mipsel-linux-gnu'
On 2016/11/21 20:08:39, Jim Stichnoth wrote:
> Is it possible to use a nacl-specific tuple for MIPS?  This is what the other
> targets do, and that may also mean that the new .bundle_align_mode directive
in
> IceTargetLoweringMIPS32.cpp would not be needed.

This change stems from the fact that when we run the crosstests in asm mode,
llvm-mc does its own sandboxing which results in duplicate instructions
generated, therefore exceeding the maximum bundle size.
We had two options, either to let subzero do the sandboxing and somehow
prevent llvm-mc from doing it, or vice versa.

We opted for the former, since letting llvm-mc do the sandboxing somehow
doesn't isolate t6, t7 and t8 registers which are used for storing masks
used in sandboxing, which in turn get used where they shouldn't be.
The way in which we accomplished that is to change the triple in this very
spot, i.e. from mipsel-nacl to mipsel-linux-gnu.
This in turn required that we generate the .bundle_align_mode directive.

We tried going the other way, but I am unaware whether there is a way to
tell llvm-mc to sequester said registers.

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

https://codereview.chromium.org/2519863002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:5847: OstreamLocker L(Ctx);
On 2016/11/21 20:08:39, Jim Stichnoth wrote:
> Just noticed this...  Use the normal pattern at the start of this method:
> 
>   if (!BuildDefs::dump())
>     return;

Done.

[Jim Stichnoth, 2016-11-22 18:25:30]

lgtm

https://codereview.chromium.org/2519863002/diff/1/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/2519863002/diff/1/pydir/crosstest.py#newcode161
pydir/crosstest.py:161: '-triple=' + ('mipsel-linux-gnu'
On 2016/11/22 11:38:13, Stefan Maksimovic wrote:
> On 2016/11/21 20:08:39, Jim Stichnoth wrote:
> > Is it possible to use a nacl-specific tuple for MIPS?  This is what the
other
> > targets do, and that may also mean that the new .bundle_align_mode directive
> in
> > IceTargetLoweringMIPS32.cpp would not be needed.
> 
> This change stems from the fact that when we run the crosstests in asm mode,
> llvm-mc does its own sandboxing which results in duplicate instructions
> generated, therefore exceeding the maximum bundle size.
> We had two options, either to let subzero do the sandboxing and somehow
> prevent llvm-mc from doing it, or vice versa.
> 
> We opted for the former, since letting llvm-mc do the sandboxing somehow
> doesn't isolate t6, t7 and t8 registers which are used for storing masks
> used in sandboxing, which in turn get used where they shouldn't be.
> The way in which we accomplished that is to change the triple in this very
> spot, i.e. from mipsel-nacl to mipsel-linux-gnu.
> This in turn required that we generate the .bundle_align_mode directive.
> 
> We tried going the other way, but I am unaware whether there is a way to
> tell llvm-mc to sequester said registers.

Ah, OK, that's very interesting.  Thanks for the explanation.

[Jim Stichnoth, 2016-11-22 18:25:41]

Description was changed from

==========
Subzero, MIPS32: Changes for improving sandbox crosstest results
==========

to

==========
Subzero, MIPS32: Changes for improving sandbox crosstest results

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-11-22 18:25:43]

Committed patchset #2 (id:20001) manually as
8208e756c4b6b488d2fc053072b579c63d5d73ae (presubmit successful).