Add hybrid assembler concept to ARM assembler.

Add hybrid assembler concept to ARM assembler.

Adds a notion of a hybrid assembler. That is, if the integrated
assembler can lower an instruction to bytes, it does. Otherwise, it
uses the standalone assembler to generate text as the placeholder for
the instruction. This is done using a textual fixup in the assembly
buffer.

The advantage of the hybrid assembler is that one can incrementally
implement the integrated assembler and still test the generated
assembly.

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=2fee2a2f3379d82e2e7a5a2de1e66a5004d50f27

[Karl, 2015-10-19 22:01:51]

Description was changed from

==========
Add hybrid assembler concept to ARM assembler.

Adds a notion of a hybrid assembler. That is, if the integrated
assembler can lower an instruction to bytes, it does. Otherwise, it
uses the standalone assembler to generate text as the placeholder for
the instruction. This is done using a textual fixup in the assembly
buffer.

The advantage of the hybrid assembler is that one can incrementally
implement the integrated assembler and still test the generated
assembly.

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

to

==========
Add hybrid assembler concept to ARM assembler.

Adds a notion of a hybrid assembler. That is, if the integrated
assembler can lower an instruction to bytes, it does. Otherwise, it
uses the standalone assembler to generate text as the placeholder for
the instruction. This is done using a textual fixup in the assembly
buffer.

The advantage of the hybrid assembler is that one can incrementally
implement the integrated assembler and still test the generated
assembly.

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

[Karl, 2015-10-19 22:01:52]

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

[Jim Stichnoth, 2015-10-20 16:28:27]

First, let me say that this is awesome, thanks!

A few questions and comments on the high-level design.

1. Does spec2k run in this mode?

2. Why do we need a separate filetype=hasm?  Could this "just work" with
filetype=iasm?  After all, iasm is only supposed to be a bridge between asm and
obj.

3. I kind of don't like the ARM assembly methods returning bool instead of void.
 It permanently exposes what is supposed to be a temporary measure toward
bringing up the assembler, and it creates asymmetry between the x86 assembler
style and ARM.  I think it might be cleaner to hold this bool status inside the
Assembler object.  Then your change like this:

-  Asm->add(getDest(), getSrc(0), getSrc(1), SetFlags, getPredicate());
+  if (!Asm->add(getDest(), getSrc(0), getSrc(1), SetFlags, getPredicate()))
+    emitUsingTextFixup(Func);

would instead look something like this:

   Asm->add(getDest(), getSrc(0), getSrc(1), SetFlags, getPredicate());
+  if (Asm->needsTextFixup())
+    emitUsingTextFixup(Func);

There would also be the issue that your nicely compact construct like:

+  if (decodeOperand(OpRd, Rd) != DecodedAsRegister)
+    return false;

would need to change to something more verbose like:

  if (decodeOperand(OpRd, Rd) != DecodedAsRegister) {
    Asm->setNeedsTextFixup();
    return;
  }

However, it looks like something like this would work and be as compact as
before:

void Assembler::setNeedsTextFixup() { ... }

...
  if (decodeOperand(OpRd, Rd) != DecodedAsRegister)
    return Asm->setNeedsTextFixup();
...

At least, g++ has no problem with a void-return-type function having a "return
other_void_function();" statement.

4. This is such a great feature that we will definitely want it for any future
targets, such as MIPS.  As such, I think some of the ARM assembler
infrastructure could be lifted into the base class.

https://chromiumcodereview.appspot.com/1418523002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1418523002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:53: std::string Message;
Can/should this be const?

[Jim Stichnoth, 2015-10-20 16:32:25]

(I had an XSRF token issue when trying to send this out the first time, so I'm
resending to be safe.)


First, let me say that this is awesome, thanks!

A few questions and comments on the high-level design.

1. Does spec2k run in this mode?

2. Why do we need a separate filetype=hasm?  Could this "just work" with
filetype=iasm?  After all, iasm is only supposed to be a bridge between asm and
obj.

3. I kind of don't like the ARM assembly methods returning bool instead of void.
 It permanently exposes what is supposed to be a temporary measure toward
bringing up the assembler, and it creates asymmetry between the x86 assembler
style and ARM.  I think it might be cleaner to hold this bool status inside the
Assembler object.  Then your change like this:

-  Asm->add(getDest(), getSrc(0), getSrc(1), SetFlags, getPredicate());
+  if (!Asm->add(getDest(), getSrc(0), getSrc(1), SetFlags, getPredicate()))
+    emitUsingTextFixup(Func);

would instead look something like this:

   Asm->add(getDest(), getSrc(0), getSrc(1), SetFlags, getPredicate());
+  if (Asm->needsTextFixup())
+    emitUsingTextFixup(Func);

There would also be the issue that your nicely compact construct like:

+  if (decodeOperand(OpRd, Rd) != DecodedAsRegister)
+    return false;

would need to change to something more verbose like:

  if (decodeOperand(OpRd, Rd) != DecodedAsRegister) {
    Asm->setNeedsTextFixup();
    return;
  }

However, it looks like something like this would work and be as compact as
before:

void Assembler::setNeedsTextFixup() { ... }

...
  if (decodeOperand(OpRd, Rd) != DecodedAsRegister)
    return Asm->setNeedsTextFixup();
...

At least, g++ has no problem with a void-return-type function having a "return
other_void_function();" statement.

4. This is such a great feature that we will definitely want it for any future
targets, such as MIPS.  As such, I think some of the ARM assembler
infrastructure could be lifted into the base class.

[Karl, 2015-10-20 21:29:31]

1) I did not run it on spec2k because I couldn't find the instructions to
download it, and build the pexes. However, to show that we are generating the
same byte sequence, I updated the literal tests to check that the disassembled
object files were the same.

2) I reorganized, based on my understanding of what you suggested.

https://chromiumcodereview.appspot.com/1418523002/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1418523002/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:53: std::string Message;
On 2015/10/20 16:28:27, stichnot wrote:
> Can/should this be const?

Done.

[Karl, 2015-10-20 21:30:43]

I'm also not convinced that we are handling branches correctly. I plan on adding
examples in the next CL.

[Jim Stichnoth, 2015-10-21 01:13:38]

Just a few nits, otherwise LGTM.

BTW I tried it out and noticed that spec2k doesn't translate - probably i64
related - but that's something for a new CL.

https://codereview.chromium.org/1418523002/diff/20001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1418523002/diff/20001/src/IceClFlags.h#newcod...
src/IceClFlags.h:255: bool AllowHybridAssembly;
Possible suggestion, if you agree.  It might be useful to have a flag to
explicitly disallow the hybrid form.  This would let you translate a big test
like spec2k with filetype=iasm and fail if you've missed any cases.  Or,
translate the cross tests that way, as the cross tests do a pretty good job of
testing vector instructions which spec2k absolutely does not do.

https://codereview.chromium.org/1418523002/diff/60001/src/IceAssembler.cpp
File src/IceAssembler.cpp (right):

https://codereview.chromium.org/1418523002/diff/60001/src/IceAssembler.cpp#ne...
src/IceAssembler.cpp:88: const intptr_t OneKB = 1024;
Make these constexprs while you're at it

https://codereview.chromium.org/1418523002/diff/60001/src/IceFixups.h
File src/IceFixups.h (right):

https://codereview.chromium.org/1418523002/diff/60001/src/IceFixups.h#newcode48
src/IceFixups.h:48: // Emits fixup, then returns the number of bytes to skip.
Use /// doxygen style comment?

https://codereview.chromium.org/1418523002/diff/60001/src/IceFixups.h#newcode58
src/IceFixups.h:58: /// Extends a fixup to be a textual. That is, it emits text
instead of a
I would say s/to be a/to be/ .

https://codereview.chromium.org/1418523002/diff/60001/src/IceFixups.h#newcode59
src/IceFixups.h:59: /// sequence of bytes. This class is used to implement
unimplemented emitIAS
maybe "as a fallback for" instead of "to implement"

https://codereview.chromium.org/1418523002/diff/60001/src/IceFixups.h#newcode61
src/IceFixups.h:61: class AssemblerTextFixup : public AssemblerFixup {
Delete default copy ctor, assignment operator, etc.

[Karl, 2015-10-22 15:19:32]

Committed patchset #5 (id:80001) manually as
2fee2a2f3379d82e2e7a5a2de1e66a5004d50f27 (presubmit successful).

[Karl, 2015-10-22 15:20:53]

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceAssembler...
File src/IceAssembler.cpp (right):

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceAssembler...
src/IceAssembler.cpp:88: const intptr_t OneKB = 1024;
On 2015/10/21 01:13:38, stichnot wrote:
> Make these constexprs while you're at it

Done.

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceFixups.h
File src/IceFixups.h (right):

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceFixups.h#...
src/IceFixups.h:48: // Emits fixup, then returns the number of bytes to skip.
On 2015/10/21 01:13:38, stichnot wrote:
> Use /// doxygen style comment?

Done.

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceFixups.h#...
src/IceFixups.h:58: /// Extends a fixup to be a textual. That is, it emits text
instead of a
On 2015/10/21 01:13:38, stichnot wrote:
> I would say s/to be a/to be/ .

Done.

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceFixups.h#...
src/IceFixups.h:59: /// sequence of bytes. This class is used to implement
unimplemented emitIAS
On 2015/10/21 01:13:38, stichnot wrote:
> maybe "as a fallback for" instead of "to implement"

Done.

https://chromiumcodereview.appspot.com/1418523002/diff/60001/src/IceFixups.h#...
src/IceFixups.h:61: class AssemblerTextFixup : public AssemblerFixup {
On 2015/10/21 01:13:38, stichnot wrote:
> Delete default copy ctor, assignment operator, etc.

Done.

[Karl, 2015-10-22 15:21:38]

Also added flag '-no-hybrid-asm' to turn off hybrid generation.