Adds the x86-64 assembler.

Adds the x86-64 assembler.

As part of this CL, x86-32 assembler tests are also introduced. They were implemented before the x86 base assembler template was modified for x86-64 support.

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

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

[John, 2015-07-21 21:25:52]

jpp@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org, stichnot@chromium.org

[John, 2015-07-21 21:27:52]

***** The x86-64 assembler is not yet implemented!!!!! *****

This cl introduces a massive amount of tests for the x86-32 (and x86-64.) Right
now, only the x86-32 tests are implemented. x86-64 (and x86-64 tests) will be
introduced in follow up patches to this CL.

[jvoung (off chromium), 2015-07-23 17:59:36]

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (left):

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:522: typename Traits::XmmRegister src, const Immediate
&mask);
oops (on the weird clustering)... is this one even used?

I imagine only the ones with the Subzero "Type Ty" as the first parameter are
actually used.

Ah, set1ps is using it. Maybe redirect w/ a dummy parameter (which is ignored
anyway)? This setup was attempting to share more of the dispatchers like
"emitIASThreeOpImmOps", where some methods like pinsr need the type while others
don't like shufps.

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:616: void pextr(Type Ty, typename Traits::GPRRegister
dst,
I think just delete it then, and we can add it back if needed?

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:794: template <class T = Traits, typename = typename
std::enable_if<!T::Is64Bit>::type>
80 col

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
80 col?

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
Default of 32-bit is interesting... I suppose that is a shorter encoding if the
operands are 32-bit or less.

https://codereview.chromium.org/1224173006/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1224173006/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:66: static const FixupKind PcRelFixup =
llvm::ELF::R_386_PC32; // TODO(jpp): ???
FYI the llvm::ELF:: stuff comes from

toolchain_build/src/llvm/include/llvm/Support/ELFRelocs/x86_64.def, for example

but TODO is good for now, until you decide which of those...

https://codereview.chromium.org/1224173006/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:256: // The Traits for lowering/insts for
x86-64 are very similar to the ones for x86-32, so these are kept here.
Just checking, but is the #if 0 necessary?

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8632Test.cpp (right):

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:92: verifyBytes<ByteCount>(codeBytes(),
0xc3);
Now that verifyBytes returns a bool, does this need to be wrapped in
ASSERT_TRUE(), or is the built-in EXPECT_EQ enough?

I suppose the reasoning is that (a) ASSERT_TRUE doesn't prevent the other TEST_F
from running, and (b) this is the last statement within the current TEST_F, so
ASSERT_TRUE won't stop it from running much more?

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:5716: verifyBytes<BytesSize>(codeBytes(),
__VA_ARGS__);                          \
Similar question about ASSERT_TRUE, except now it's in the middle of a TEST_F.

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:7282: 
Uh... haven't read all this. I'm assuming this is sampling things from certain
equivalence classes of instructions and not testing things that may be
redundant.

[John, 2015-07-24 21:49:30]

OK, so the assembler is "done." We can now use the extra registers, but i64
support is still not implemented. I64 support will be added after the x86-64
target lowering is implemented.

This CL looks huge, but it is mostly testing -- and I am glad I wrote them: I
broken the x86-32 tests several times. Plus, the x86-32 tests were copied and
used for testing the x86-64 assembler. At this point, I am confident the
assembler emits valid x86-64 code (I am not so sure about invalid code.)

If you have to prioritize, please focus on src/IceAssemblerX86Base[Impl]?.h

[Jim Stichnoth, 2015-07-26 15:31:11]

I don't really know how to go about reviewing the two test files...  But here's
something fun to think about.  Some instructions have two (or more) valid
encodings.  E.g.,
  mov eax, ecx
can be encoded as
  mov r32, r/m32
or
  mov r/m32, r32

We noticed this when comparing the objdump output of Subzero-compiled programs
using filetype=asm versus filetype=iasm or filetype=obj (apparently the llvm-mc
assembler makes different choices than Subzero).

But I guess that choice is made at a higher level than the assembler, and
shouldn't affect the assembler tests.

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:976: // emitRex is used for emitting a Rex prefix for
an address and a GPR. The
Did you swap the description of emitRex and emitRexRB?

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base...
src/IceAssemblerX86BaseImpl.h:186: emitRex(/*Any non-i64 type*/ IceType_i32,
address,
It seems that most or all instances of IceType_i32 in this file really mean
"some type value that leads to certain default behavior in the rex byte"
(specifically non i64/i8/i1 I think).  Does it make sense to use some constexpr
or const static field for these instances?

https://codereview.chromium.org/1224173006/diff/90001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
80-col

https://codereview.chromium.org/1224173006/diff/90001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:25: X(Reg_eax,  =  0, "rax", "eax" ,  "ax" , "al"  , 1, 0,
0, 0, 1, 0) \
align the trailing backslashes

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.def (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.def:18: #define FCMPX8664_TABLE                      
                       \
I would expect that the FCMP and ICMP tables for x86-32 and x86-64 would be
practically identical.  The only difference is that x86-64 has no need for the
C1_64, C2_64, and C3_64 columns of the icmp table.

What do you think about not duplicating the file, and just s/X8632/X86/ ?

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:278: #if 0
Why keep the "#if 0" commented-out code around?

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:279: // The Traits for lowering/insts for
x86-64 are very similar to the ones for x86-32, so these are kept here.
80-col

https://codereview.chromium.org/1224173006/diff/90001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8664Test.cpp (right):

https://codereview.chromium.org/1224173006/diff/90001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8664Test.cpp:1333: //       movusp %xmm3,  {152 +
$ScratchpadOffset}(%rbp)
movups not movusp, right?

[John, 2015-07-27 20:35:59]

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (left):

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:522: typename Traits::XmmRegister src, const Immediate
&mask);
On 2015/07/23 17:59:36, jvoung wrote:
> oops (on the weird clustering)... is this one even used?
> 
> I imagine only the ones with the Subzero "Type Ty" as the first parameter are
> actually used.
> 
> Ah, set1ps is using it. Maybe redirect w/ a dummy parameter (which is ignored
> anyway)? This setup was attempting to share more of the dispatchers like
> "emitIASThreeOpImmOps", where some methods like pinsr need the type while
others
> don't like shufps.

Yeah, initially I thought about removing the irregular shups method, but then I
simply decided to keep it as shufpd was also missing the type argument.

I went ahead and removed them both as they are not being used anywhere else.

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1224173006/diff/60001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:794: template <class T = Traits, typename = typename
std::enable_if<!T::Is64Bit>::type>
On 2015/07/23 17:59:35, jvoung wrote:
> 80 col

Done.

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
On 2015/07/23 17:59:36, jvoung wrote:
> Default of 32-bit is interesting... I suppose that is a shorter encoding if
the
> operands are 32-bit or less.

Yup, if you need 64 operands than you need a rex prefix -- we probably don't
need it anyways.

More importantly, by using the 32-bit names instructions with implicit operands
(e.g., shl %cl, %reg) don't need special handling for each target.

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
On 2015/07/23 17:59:36, jvoung wrote:
> 80 col?

Done.

https://codereview.chromium.org/1224173006/diff/60001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
On 2015/07/23 17:59:36, jvoung wrote:
> 80 col?

Done.

https://codereview.chromium.org/1224173006/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1224173006/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:66: static const FixupKind PcRelFixup =
llvm::ELF::R_386_PC32; // TODO(jpp): ???
On 2015/07/23 17:59:36, jvoung wrote:
> FYI the llvm::ELF:: stuff comes from
> 
> toolchain_build/src/llvm/include/llvm/Support/ELFRelocs/x86_64.def, for
example
> 
> but TODO is good for now, until you decide which of those...

Thanks for the pointer.

https://codereview.chromium.org/1224173006/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:256: // The Traits for lowering/insts for
x86-64 are very similar to the ones for x86-32, so these are kept here.
On 2015/07/23 17:59:36, jvoung wrote:
> Just checking, but is the #if 0 necessary?

It would help me out to keep implementing the backend alongside with the code
reviews, but not that big of a deal. Removed.

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8632Test.cpp (right):

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:92: verifyBytes<ByteCount>(codeBytes(),
0xc3);
On 2015/07/23 17:59:36, jvoung wrote:
> Now that verifyBytes returns a bool, does this need to be wrapped in
> ASSERT_TRUE(), or is the built-in EXPECT_EQ enough?
> 
> I suppose the reasoning is that (a) ASSERT_TRUE doesn't prevent the other
TEST_F
> from running, and (b) this is the last statement within the current TEST_F, so
> ASSERT_TRUE won't stop it from running much more?

See the comment below.

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:5716: verifyBytes<BytesSize>(codeBytes(),
__VA_ARGS__);                          \
On 2015/07/23 17:59:36, jvoung wrote:
> Similar question about ASSERT_TRUE, except now it's in the middle of a TEST_F.

Having the ASSERT_TRUE outside verifyFunction helps debugging the test when it
failed as the error reports a line that is not within verifyBytes. I added the
assertion to all uses of verifyBytes in tests with multiple calls to it.

https://codereview.chromium.org/1224173006/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:7282: 
On 2015/07/23 17:59:36, jvoung wrote:
> Uh... haven't read all this. I'm assuming this is sampling things from certain
> equivalence classes of instructions and not testing things that may be
> redundant.

Well, these are just dumb tests -- I went over the methods in the assembler
class and tested all of them. Implementing these was painful, but after they
were done adding REX support was fairly painless.

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:976: // emitRex is used for emitting a Rex prefix for
an address and a GPR. The
On 2015/07/26 15:31:11, stichnot wrote:
> Did you swap the description of emitRex and emitRexRB?

Yes I did. Done.

On a related note, I changed this quite a lot in this new implementation --
Instead of three different methods I now have a centralized "rex-emitter"
method. I am not sure that helps with the readability, but it is (IMO) more
clear when a REX prefix is emitted/needed.

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceAssemblerX86Base...
src/IceAssemblerX86BaseImpl.h:186: emitRex(/*Any non-i64 type*/ IceType_i32,
address,
On 2015/07/26 15:31:11, stichnot wrote:
> It seems that most or all instances of IceType_i32 in this file really mean
> "some type value that leads to certain default behavior in the rex byte"
> (specifically non i64/i8/i1 I think).  Does it make sense to use some
constexpr
> or const static field for these instances?

Yes, it does. I added two named constants (IceType_Irrelevant, and
Encoded_Reg_Irrelevant) to the Assembler.

https://codereview.chromium.org/1224173006/diff/90001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:20: //       (2) when generating code, subzero defaults to
using 32 bit registers, so using the 32 bit register name would hopefully make
this design more explicit.
On 2015/07/26 15:31:11, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1224173006/diff/90001/src/IceInstX8664.def#ne...
src/IceInstX8664.def:25: X(Reg_eax,  =  0, "rax", "eax" ,  "ax" , "al"  , 1, 0,
0, 0, 1, 0) \
On 2015/07/26 15:31:11, stichnot wrote:
> align the trailing backslashes

Done.

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.def (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.def:18: #define FCMPX8664_TABLE                      
                       \
On 2015/07/26 15:31:11, stichnot wrote:
> I would expect that the FCMP and ICMP tables for x86-32 and x86-64 would be
> practically identical.  The only difference is that x86-64 has no need for the
> C1_64, C2_64, and C3_64 columns of the icmp table.
> 
> What do you think about not duplicating the file, and just s/X8632/X86/ ?

I actually don't need this file yet, so I just removed it.

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:278: #if 0
On 2015/07/26 15:31:11, stichnot wrote:
> Why keep the "#if 0" commented-out code around?

Because it would make my life a little bit easier when merging work done in
parallel with this code review -- there's also precedent in subzero for keeping
thins that aren't needed just yet but might very soon. Oh, well, not that big of
a deal. Done.

https://codereview.chromium.org/1224173006/diff/90001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:279: // The Traits for lowering/insts for
x86-64 are very similar to the ones for x86-32, so these are kept here.
On 2015/07/26 15:31:11, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1224173006/diff/90001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8664Test.cpp (right):

https://codereview.chromium.org/1224173006/diff/90001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8664Test.cpp:1333: //       movusp %xmm3,  {152 +
$ScratchpadOffset}(%rbp)
On 2015/07/26 15:31:11, stichnot wrote:
> movups not movusp, right?

Done.

https://codereview.chromium.org/1224173006/diff/100001/src/IceRegistersX8664.h
File src/IceRegistersX8664.h (left):

https://codereview.chromium.org/1224173006/diff/100001/src/IceRegistersX8664....
src/IceRegistersX8664.h:84: if (RegNum == Reg_ah)
This is not going to work for X86-64. Accessing AH would mean constraining the
other src operand (if it is a register) to al, bl, cl, or dl. Otherwise, really
bad things would happen. I am tempted to force ah access to require a 16 bit mov
+ 16 bit shr.

[Jim Stichnoth, 2015-07-28 21:56:34]

otherwise lgtm

https://codereview.chromium.org/1224173006/diff/110001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1224173006/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:317: void call(const ConstantRelocatable *label); //
not testable.
Not testable, or not currently tested?  (not sure what the former would actually
mean)

https://codereview.chromium.org/1224173006/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:894: static constexpr Type IceType_Irrelevant =
IceType_i32;
I don't really like the reuse of the IceType_ prefix here.  I would expect to
find its definition among the rest of the IceType_ enum values, yet it's
x86-specific.

How about something like Type_Irrelevant or X86Type_Irrelevant or
RexTypeIrrelevant?

(Actually, I like RexTypeIrrelevant and maybe RexTypeForceRexW given that they
are only used as arguments to the emitRex routines.)

[John, 2015-07-28 23:28:12]

Committed patchset #11 (id:140001) manually as
2fea26cae5a6b140c67e18aec3dcf645a16694d5 (presubmit successful).

[John, 2015-07-28 23:33:46]

https://codereview.chromium.org/1224173006/diff/110001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1224173006/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:317: void call(const ConstantRelocatable *label); //
not testable.
On 2015/07/28 21:56:33, stichnot wrote:
> Not testable, or not currently tested?  (not sure what the former would
actually
> mean)

Not testable: creating a ConstantRelocatable is impossible without a
GlobalContext. :(

https://codereview.chromium.org/1224173006/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:894: static constexpr Type IceType_Irrelevant =
IceType_i32;
On 2015/07/28 21:56:33, stichnot wrote:
> I don't really like the reuse of the IceType_ prefix here.  I would expect to
> find its definition among the rest of the IceType_ enum values, yet it's
> x86-specific.
> 
> How about something like Type_Irrelevant or X86Type_Irrelevant or
> RexTypeIrrelevant?
> 
> (Actually, I like RexTypeIrrelevant and maybe RexTypeForceRexW given that they
> are only used as arguments to the emitRex routines.)

These aren't directly related to REX -- and yet they are. I could not think of
any better names than IceType_* and Encoded_Reg_*, so I'll just rename them to

IceType_Irrelevant --> RexTypeIrrelevant; and
Encoded_Reg_Irrelevant --> RexRegIrrelevant.

[Jim Stichnoth, 2015-07-30 07:03:34]

I can't build the new assembler tests on my laptop.  Clang runs out of memory. 
My chrome tabs are all very sad.  Hopefully something can be done about this...