Add workaround to allow testing of ARM integrated assembler.

Add workaround to allow testing of ARM integrated assembler.

It turns out that there are several instruction in the ARM integrated
assembler that do not get translated correctly. This results in the
spec2k tests not being compilable.

To workaround this problem, this CL adds a (temporary) flag that allows all translations to be applied by the integrated assembler. When this flag is false (the default) only correctly working translations to be applied by the integrated assembler. This allows lit tests to still be applied to the correct portions of broken translations.

This CL also fixes a bug with local (instruction) labels that did not
generate a corresponding label to the -filetype=iasm assembly file.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2015-11-02 22:48:45]

Description was changed from

==========
Add workaround to allow testing of ARM integrated assembler.

It turns out that there are several instruction in the ARM integrated
assembler that do not get translated correctly. This results in the
spec2k tests not being compilable.

To workaround this problem, this CL adds the make argument IASM=SAFE=1
to compile pnaclsz in such a way that only working (i.e. safe)
assembly instructions are lowered to bytes.

This CL also fixes a bug with local (instruction) labels that did not
generate a corresponding label to the -filetype=iasm assembly file.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add workaround to allow testing of ARM integrated assembler.

It turns out that there are several instruction in the ARM integrated
assembler that do not get translated correctly. This results in the
spec2k tests not being compilable.

To workaround this problem, this CL adds the make argument IASM=SAFE=1
to compile pnaclsz in such a way that only working (i.e. safe)
assembly instructions are lowered to bytes.

This CL also fixes a bug with local (instruction) labels that did not
generate a corresponding label to the -filetype=iasm assembly file.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-11-02 22:48:45]

kschimpf@google.com changed reviewers:
+ jpp@chromium.org, sehr@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2015-11-03 00:21:43]

Sorry to have to say this, but yuck!  Could you work toward a fix instead of a
temporary workaround?  I'm happy to continue reviewing new iasm patches while
this gets patiently worked out in the background.

I say this without any knowledge of the pain you've already gone through
debugging this, so maybe we should first talk through the high-level strategy on
this in person.

BTW, I get a number of "make check-lit" failures with this patch.

https://codereview.chromium.org/1424923005/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1424923005/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:66: static InstARM32::InstKindARM32 SafeIASMInsts[] = {
I think a blacklist of unsafe instructions would be much better than a whitelist
of safe instructions, because it's easier (commented out instructions
notwithstanding) to tell how close to completion you are.

https://codereview.chromium.org/1424923005/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:83: for (unsigned i = 0; i <
llvm::array_lengthof(SafeIASMInsts); ++i) {
I realize it's only temporary, but this linear search offends my sense of
efficiency. :)

I think you could do something like this:

static std::unordered_set<uint32_t> SafeIASMInsts(
    {// TODO(kschimpf): Figure out why commented out instructions are not
     // (always)
     // translated correctly.
     InstARM32::Adc, InstARM32::Add, InstARM32::And,
     // InstARM32::Br,
     InstARM32::Cmp, InstARM32::Eor, InstARM32::Ldr,
     // InstARM32::Mov,
     // InstARM32::Movw,
     // InstARM32::Movt,
     InstARM32::Mul, InstARM32::Orr, InstARM32::Sbc, InstARM32::Sdiv,
     InstARM32::Str, InstARM32::Sub, InstARM32::Udiv, InstARM32::Ret,
     InstARM32::Label});

} // end of anonymous namespace

bool InstARM32::isIasSafeImpl(InstKind Kind) const {
  return SafeIASMInsts.find((uint32_t)Kind) != SafeIASMInsts.end();
}

[Karl, 2015-11-03 18:56:11]

Rewrote this CL to reduce yukiness. By default, only fully working translations
are lowered by the integrated assembler. The remaining are only done if flag
'-unsafe-ias' is added to the command-line of pnaclsz.

Also modifies cross-tests to use hybrid (safe) integrated assembler by default
for ARM.

https://codereview.chromium.org/1424923005/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1424923005/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:66: static InstARM32::InstKindARM32 SafeIASMInsts[] = {
On 2015/11/03 00:21:42, stichnot wrote:
> I think a blacklist of unsafe instructions would be much better than a
whitelist
> of safe instructions, because it's easier (commented out instructions
> notwithstanding) to tell how close to completion you are.

Done.

https://codereview.chromium.org/1424923005/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:83: for (unsigned i = 0; i <
llvm::array_lengthof(SafeIASMInsts); ++i) {
On 2015/11/03 00:21:42, stichnot wrote:
> I realize it's only temporary, but this linear search offends my sense of
> efficiency. :)
> 
> I think you could do something like this:
> 
> static std::unordered_set<uint32_t> SafeIASMInsts(
>     {// TODO(kschimpf): Figure out why commented out instructions are not
>      // (always)
>      // translated correctly.
>      InstARM32::Adc, InstARM32::Add, InstARM32::And,
>      // InstARM32::Br,
>      InstARM32::Cmp, InstARM32::Eor, InstARM32::Ldr,
>      // InstARM32::Mov,
>      // InstARM32::Movw,
>      // InstARM32::Movt,
>      InstARM32::Mul, InstARM32::Orr, InstARM32::Sbc, InstARM32::Sdiv,
>      InstARM32::Str, InstARM32::Sub, InstARM32::Udiv, InstARM32::Ret,
>      InstARM32::Label});
> 
> } // end of anonymous namespace
> 
> bool InstARM32::isIasSafeImpl(InstKind Kind) const {
>   return SafeIASMInsts.find((uint32_t)Kind) != SafeIASMInsts.end();
> }

Actually, reducing the footprint by moving this to a simpler test in each of
these four instructions.

[Jim Stichnoth, 2015-11-04 01:01:10]

LGTM, and I hope this whole thing is very short-lived. :)

https://codereview.chromium.org/1424923005/diff/80001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1424923005/diff/80001/pydir/crosstest_generat...
pydir/crosstest_generator.py:69: # ARM doesn't have a complete integrated
assembler yet.
I would change this comment to say that ARM doesn't have an ELF writer yet.  The
default in crosstest.py is iasm, but that could be changed to obj which would
break arm32 without the explicit --filetype=iasm.

https://codereview.chromium.org/1424923005/diff/80001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1424923005/diff/80001/src/IceClFlags.h#newcod...
src/IceClFlags.h:262: bool AllowUnsafeIas;
Can we have a TODO to nuke this flag?

[Karl, 2015-11-04 15:53:50]

Description was changed from

==========
Add workaround to allow testing of ARM integrated assembler.

It turns out that there are several instruction in the ARM integrated
assembler that do not get translated correctly. This results in the
spec2k tests not being compilable.

To workaround this problem, this CL adds the make argument IASM=SAFE=1
to compile pnaclsz in such a way that only working (i.e. safe)
assembly instructions are lowered to bytes.

This CL also fixes a bug with local (instruction) labels that did not
generate a corresponding label to the -filetype=iasm assembly file.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add workaround to allow testing of ARM integrated assembler.

It turns out that there are several instruction in the ARM integrated
assembler that do not get translated correctly. This results in the
spec2k tests not being compilable.

To workaround this problem, this CL adds a (temporary) flag that allows all
translations to be applied by the integrated assembler. When this flag is false
(the default) only correctly working translations to be applied by the
integrated assembler. This allows lit tests to still be applied to the correct
portions of broken translations.

This CL also fixes a bug with local (instruction) labels that did not
generate a corresponding label to the -filetype=iasm assembly file.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-11-04 16:02:12]

Committed patchset #6 (id:100001) manually as
cb504ca52cb41c781e8642ea652e7db08ec27f9a (presubmit successful).

[Karl, 2015-11-04 16:02:40]

https://codereview.chromium.org/1424923005/diff/80001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1424923005/diff/80001/pydir/crosstest_generat...
pydir/crosstest_generator.py:69: # ARM doesn't have a complete integrated
assembler yet.
On 2015/11/04 01:01:09, stichnot wrote:
> I would change this comment to say that ARM doesn't have an ELF writer yet. 
The
> default in crosstest.py is iasm, but that could be changed to obj which would
> break arm32 without the explicit --filetype=iasm.

Done.

https://codereview.chromium.org/1424923005/diff/80001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1424923005/diff/80001/src/IceClFlags.h#newcod...
src/IceClFlags.h:262: bool AllowUnsafeIas;
On 2015/11/04 01:01:09, stichnot wrote:
> Can we have a TODO to nuke this flag?

Done.