[SubZero] Generate ELF output for MIPS

[SubZero] Generate ELF output for MIPS

This patch adds encoding of few arithmetic and branch instructions.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-08-19 06:25:53]

Description was changed from

==========
[SubZero] Generate ELF output for MIPS

This patch adds encoding of few arithmetic and branch instructions.
==========

to

==========
[SubZero] Generate ELF output for MIPS

This patch adds encoding of few arithmetic and branch instructions.
==========

[jaydeep.patil, 2016-08-19 06:25:53]

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

[Jim Stichnoth, 2016-08-19 17:12:21]

This is great to see!

The main thing here is that this CL must have tests.

There are a couple ways to approach this.  If you haven't already done so, you
may want to want to look at Karl's CLs, specifically the ones with some
permutation of "integrated ARM assembler" in the title.

https://codereview.chromium.org/search?closed=2&owner=kschimpf%40google.com&r...

There were a couple strategies in play during the development of the ARM
assembler in Subzero.  First, each new instruction encoding was meticulously
tested via tests_lit/assembler/arm32/ list tests.  I would say this level of
detail is optional given your MIPS expertise, as opposed to us basically
learning ARM as we developed the assembler.  One special thing to point out for
ARM is that sometimes register encodings are split into discontiguous bits of
the instruction encoding.  To test this properly, we had to force the exclusion
of low-numbered registers so that the high-order bits wouldn't always be zero. 
I don't know if this is the case for MIPS encodings.

Second, we developed the "hybrid assembly" strategy.  Here's an explanation. 
Subzero has 3 -filetype= options: asm, iasm, and obj.  "asm" is the pure textual
assembly that you've been using so far.  It's relatively easy to produce asm
output, and to read the output, and is the best choice for initial bring-up. 
"obj" is the production version, which incorporates an integrated assembler with
relocations, symbols, and other aspects of ELF generation.  "iasm" is the
intermediate step - we use the integrated assembler but output the instruction
bytes as textual ".byte" lines where possible, and then use the same
filetype=asm assembler to create the ELF object.

"Hybrid assembly" is a combination of filetype=asm and filetype=iasm that allows
integrated assembler support to be gradually phased in.  If we know how to
encode e.g. an "add" instruction, we emit it as ".byte" lines, and for
not-yet-supported instructions, we use the normal filetype=asm textual assembly.
 In this case we can test the "add" instruction encoding even if we haven't yet
implemented other instructions involved in e.g. prolog or epilog generation.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp
File src/IceAssemblerMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:1: //===- subzero/src/IceAssemblerMIPS32.cpp -
Assembler for MIPS32 --*- C++ -*-===//
Fix 80-col here.  May be easiest to change the description to "MIPS32 Assembler"
and then fix the '=' string as needed.

Also, I happened to notice the first line of IceInstMIPS32.h is not perfectly
aligned, so you could fix that as well. :)

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:3: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
For the x86 and ARM32 integrated assemblers, we started with the Dart VM's
assembler code and tweaked it to our interface.

https://github.com/dart-lang/sdk/tree/master/runtime/vm

It doesn't look like you're using the Dart code in this file, and if that's
true, you should remove this copyright text and replace it with the standard
Subzero boilerplate.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:25: #include "IceCfgNode.h"
Can these be sorted alphabetically?

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:33: //using WordType = uint32_t;
Remove commented-out code?

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:60: const uint8_t TrapBytesRaw[] = {0x00, 0x00, 0x00,
0x34};
Can this be constexpr?

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:100: if (!L) {
L != nullptr

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:122: assert(Utils::IsAligned(Offset, 4) &&
Instead of:
  assert(A & B);
use:
  assert(A);
  assert(B);
for more precise assertion failures.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:199: IValueT Rt = encodeGPRegister(OpRt, "Rt",
InsnName);
const IValueT ...

here and wherever reasonable below.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:241: IValueT Opcode = 0x24000000;
Here and below, for the Opcode declarations, use static constexpr when possible.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:344: break;
Is Opcode==0 ever valid here?  If not, to be safe, add some hard-failure here,
or else check after the switch statement for Opcode!=0.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:387: emitBr(CondMIPS32::AL, nullptr, nullptr, Dest);
I think it would be easier to read like this:

static constexpr Operand *OpRxNone = nullptr;
emitBr(CondMIPS32::AL, OpRxNone, OpRxNone, Dest);

Otherwise I definitely have to look back to the declaration/definition of emitBr
to see what those nullptr args mean.

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:423: 
remove trailing whitespace (blank line) to keep git happy

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.h
File src/IceAssemblerMIPS32.h (right):

https://codereview.chromium.org/2259983004/diff/1/src/IceAssemblerMIPS32.h#ne...
src/IceAssemblerMIPS32.h:109: void bcc(const CondMIPS32::Cond Cond, const
Operand *OpRs, const Operand *OpRt, Label *TargetLabel);
80-col

This is usually automatically fixed by running:
  make -f Makefile.standalone format
which should always be done before committing or uploading a patchset.

But note that "make format" deliberately doesn't apply to .def files, because in
those files we often try to carefully arrange things in a grid layout, and
clang-format usually ruins that.

[jaydeep.patil, 2016-08-24 05:27:22]

Addressed review comments.

[jaydeep.patil, 2016-08-24 05:37:08]

Adding Rich Fuhler

[Jim Stichnoth, 2016-08-24 15:37:39]

https://codereview.chromium.org/2259983004/diff/20001/src/IceAssemblerMIPS32.cpp
File src/IceAssemblerMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/20001/src/IceAssemblerMIPS32....
src/IceAssemblerMIPS32.cpp:1: //===- subzero/src/IceAssemblerMIPS32.cpp - MIPS32
Assembler --*- C++ -*-====//
I'm being super pedantic here, sorry...

The LLVM style that these comments are based on puts the last character in
column 80.  Here and below, your last character is in column 79.

Also, they always use 3 '=' characters, adding extra '-' characters as needed. 
(Though sometimes I've noticed them removing '=' characters if absolutely
necessary.)

So in this first line, I would expect it to end:

  ... Assembler ----*- C++ -*-===//

Actually, I didn't notice this before -- the "*- C++ -*", which is a hint for
emacs, is only needed for .h files.  So this line would end more like:

  ... Assembler --------------===//

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:756: Asm->addiu(getDest(), getSrc(0), (const
uint32_t)Imm);
Isn't the Imm field already declared as const uint32_t?

If so, this cast shouldn't be necessary?

If the cast really is needed (and surely the "const" part is not needed), then
please use the appropriate C++-style cast, e.g. reinterpret_cast<>.

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1: //===- subzero/src/IceInstMIPS32.h - MIPS32 machine
instrs --*- C++ -*-======//
Add extra '-' characters for padding instead of '='.

https://codereview.chromium.org/2259983004/diff/20001/tests_lit/assembler/mip...
File tests_lit/assembler/mips32/encoding_test_01.ll (right):

https://codereview.chromium.org/2259983004/diff/20001/tests_lit/assembler/mip...
tests_lit/assembler/mips32/encoding_test_01.ll:1: ; Test encoding of MIPS32
arithmetic instructions
Can you name the file something more indicative of its content?
e.g. encoding_test_arith.ll

It's probably fine to keep the less descriptive function names within the file.

[Jim Stichnoth, 2016-08-24 15:40:40]

BTW, it's customary to explicitly address each comment, by clicking the "Reply"
link at the bottom of the comment and entering some reply text, or clicking the
"Done" or "Ack" links which insert standard replies.

[jaydeep.patil, 2016-08-25 04:39:08]

https://codereview.chromium.org/2259983004/diff/20001/src/IceAssemblerMIPS32.cpp
File src/IceAssemblerMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/20001/src/IceAssemblerMIPS32....
src/IceAssemblerMIPS32.cpp:1: //===- subzero/src/IceAssemblerMIPS32.cpp - MIPS32
Assembler --*- C++ -*-====//
On 2016/08/24 15:37:38, stichnot wrote:
> I'm being super pedantic here, sorry...
> 
> The LLVM style that these comments are based on puts the last character in
> column 80.  Here and below, your last character is in column 79.
> 
> Also, they always use 3 '=' characters, adding extra '-' characters as needed.

> (Though sometimes I've noticed them removing '=' characters if absolutely
> necessary.)
> 
> So in this first line, I would expect it to end:
> 
>   ... Assembler ----*- C++ -*-===//
> 
> Actually, I didn't notice this before -- the "*- C++ -*", which is a hint for
> emacs, is only needed for .h files.  So this line would end more like:
> 
>   ... Assembler --------------===//

Done.

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:756: Asm->addiu(getDest(), getSrc(0), (const
uint32_t)Imm);
On 2016/08/24 15:37:39, stichnot wrote:
> Isn't the Imm field already declared as const uint32_t?
> 
> If so, this cast shouldn't be necessary?
> 
> If the cast really is needed (and surely the "const" part is not needed), then
> please use the appropriate C++-style cast, e.g. reinterpret_cast<>.

Done.

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2259983004/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:1: //===- subzero/src/IceInstMIPS32.h - MIPS32 machine
instrs --*- C++ -*-======//
On 2016/08/24 15:37:39, stichnot wrote:
> Add extra '-' characters for padding instead of '='.

Done.

[Jim Stichnoth, 2016-08-25 12:37:05]

https://codereview.chromium.org/2259983004/diff/40001/src/IceAssemblerMIPS32.h
File src/IceAssemblerMIPS32.h (right):

https://codereview.chromium.org/2259983004/diff/40001/src/IceAssemblerMIPS32....
src/IceAssemblerMIPS32.h:3: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
Remove this Dart comment?

[jaydeep.patil, 2016-08-26 03:04:53]

https://codereview.chromium.org/2259983004/diff/40001/src/IceAssemblerMIPS32.h
File src/IceAssemblerMIPS32.h (right):

https://codereview.chromium.org/2259983004/diff/40001/src/IceAssemblerMIPS32....
src/IceAssemblerMIPS32.h:3: // Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
On 2016/08/25 12:37:05, stichnot wrote:
> Remove this Dart comment?

Done.

[Jim Stichnoth, 2016-08-26 13:15:03]

I'm not sure what exactly has changed, but 2 of the lit tests are now failing,
compared to what is currently in master: 64bit.pnacl.ll and int-arg.ll.

It seems to be objdump's choice of disassembly.

E.g. in int-arg.ll we are expecting:
  add v0,zero,a0
but objdump is producing:
  move v0,a0

I tried changing GetObjdumpCmd() in utils.py to add "-M no-aliases", but that
doesn't fix it, instead we get:
  addu v0,a0,zero
Note "addu" instead of "add", and the different operand order.  Also, a number
of other tests fail with that.

https://codereview.chromium.org/2259983004/diff/60001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/60001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:516: auto *RA = llvm::cast<Variable>(getSrc(0));
This actually causes a compilation error in a MINIMAL=1 build, because when
assertions are disabled, there are no uses of RA.  The fix is to add:
  (void)RA;

[jaydeep.patil, 2016-08-29 04:20:53]

https://codereview.chromium.org/2259983004/diff/60001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2259983004/diff/60001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:516: auto *RA = llvm::cast<Variable>(getSrc(0));
On 2016/08/26 13:15:03, stichnot wrote:
> This actually causes a compilation error in a MINIMAL=1 build, because when
> assertions are disabled, there are no uses of RA.  The fix is to add:
>   (void)RA;

Done.

[jaydeep.patil, 2016-08-29 04:24:06]

On 2016/08/26 13:15:03, stichnot wrote:
> I'm not sure what exactly has changed, but 2 of the lit tests are now failing,
> compared to what is currently in master: 64bit.pnacl.ll and int-arg.ll.
> 
> It seems to be objdump's choice of disassembly.
> 
> E.g. in int-arg.ll we are expecting:
>   add v0,zero,a0
> but objdump is producing:
>   move v0,a0
> 
> I tried changing GetObjdumpCmd() in utils.py to add "-M no-aliases", but that
> doesn't fix it, instead we get:
>   addu v0,a0,zero
> Note "addu" instead of "add", and the different operand order.  Also, a number
> of other tests fail with that.
> 
> https://codereview.chromium.org/2259983004/diff/60001/src/IceInstMIPS32.cpp
> File src/IceInstMIPS32.cpp (right):
> 
>
https://codereview.chromium.org/2259983004/diff/60001/src/IceInstMIPS32.cpp#n...
> src/IceInstMIPS32.cpp:516: auto *RA = llvm::cast<Variable>(getSrc(0));
> This actually causes a compilation error in a MINIMAL=1 build, because when
> assertions are disabled, there are no uses of RA.  The fix is to add:
>   (void)RA;

Move is pseudo instruction used for register to register copy. It is encoded as
add with Src0 as $0. I have fixed the tests to use move instead of add.

[jaydeep.patil, 2016-08-29 04:27:21]

Corrected tests for reg-to-reg move insn

[Jim Stichnoth, 2016-08-29 12:13:56]

lgtm

[Jim Stichnoth, 2016-08-29 12:14:09]

Description was changed from

==========
[SubZero] Generate ELF output for MIPS

This patch adds encoding of few arithmetic and branch instructions.
==========

to

==========
[SubZero] Generate ELF output for MIPS

This patch adds encoding of few arithmetic and branch instructions.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-08-29 12:14:11]

Committed patchset #5 (id:80001) manually as
135f5dbff28dfe8bb2f33363956eb7ea5f236ff9 (presubmit successful).