Subzero. x8664. Resurrects the Target.

Subzero. x8664. Resurrects the Target.

After a hiatus while x32 was not available in nacl's llvm, x8664 is
being revived. Rejoice!

The Target is now back to where it was before: the crosstests pass, and
SPEC2k builds and verifies.

Makefile.standalone still has the crosstests for x8664 disabled while
we wait for all the plumbing that's needed for x32 support on nacl's
toolchain to be available.

BUG= https://bugs.chromium.org/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=3c275ce1e832ba9ccfb730c4235db786cf080465

[John, 2015-12-18 18:51:27]

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

[Jim Stichnoth, 2015-12-20 19:27:38]

https://codereview.chromium.org/1537703002/diff/220001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1537703002/diff/220001/Makefile.standalone#ne...
Makefile.standalone:405: -e x8632,sandbox,sse4.1,Om1 \
Why is this x8632 test now excluded?

https://codereview.chromium.org/1537703002/diff/220001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/1537703002/diff/220001/pydir/szbuild.py#newco...
pydir/szbuild.py:244: 'x8664': 'i86_64-nacl' if args.sandbox else
'x86_64-linux-gnux32',
i86_64?

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:921: template <typename T = Traits>
Could you do something like this instead?

void emitAddrSizeOverridePrefix() {
  if (Traits::Is64Bit) {
    static constexpr uint8_t AddrSizeOverridePrefix = 0x67;
    emitUint8(AddrSizeOverridePrefix);
  }
}

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:979: // possible to distinguish between the the "new"
low byte registers
the the

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:982: static uint32_t Encoded_ah = 0x04;
constexpr

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:984: isByteSizedType(Ty) && (Reg != Encoded_ah);
Could you use something like Traits::RegisterSet::Encoded_ah instead?

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp
File src/IceInstX8664.cpp (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:111: // physical register (esp or ebp), and update the
Offset.
Should that be rsp/rbp?

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:124: if (getOffset() == 0 && Disp == 0) {
Here and below, I think getOffset() should be compared against nullptr instead
of 0.

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:219: (void)Target;
remove this

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:965: //   mov ecx, ecx ==> reduntant, and dangerous in
x86-64. i64 zexting
redundant

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:201: constexpr uint16_t Encoded_rAX = 0;
It seems odd to capitalize AX here.

Also, don't you need to cast it to void for a NOASSERT/MINIMAL build?

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1513: // assert(DestReg ==
InstX86Base<Machine>::Traits::RegisterSet::Reg_edx);
There should be *some* assert here, right?

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:463: assert(static_cast<int32_t>(Canonical) ==
Traits::getBaseReg(Canonical));
I think Canonical can/should be changed to int32_t (also in the x8632 version),
and then the clumsy cast could go away.

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2849: assert(!T::Is64Bit);
This assert seems completely unnecessary given the std::enable_if above.

https://codereview.chromium.org/1537703002/diff/220001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceTypes.def#newco...
src/IceTypes.def:25: X(Target_X8664, "x86-64", false,  EM_X86_64,  0)           
                  \
realign the columns

[John, 2015-12-21 13:41:32]

https://codereview.chromium.org/1537703002/diff/220001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1537703002/diff/220001/Makefile.standalone#ne...
Makefile.standalone:405: -e x8632,sandbox,sse4.1,Om1 \
On 2015/12/20 19:27:37, stichnot wrote:
> Why is this x8632 test now excluded?

My bad -- I wanted to say x8664.

https://codereview.chromium.org/1537703002/diff/220001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/1537703002/diff/220001/pydir/szbuild.py#newco...
pydir/szbuild.py:244: 'x8664': 'i86_64-nacl' if args.sandbox else
'x86_64-linux-gnux32',
On 2015/12/20 19:27:37, stichnot wrote:
> i86_64?

Yes, that's the son of x86_64 and i686. :)

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:921: template <typename T = Traits>
On 2015/12/20 19:27:37, stichnot wrote:
> Could you do something like this instead?
> 
> void emitAddrSizeOverridePrefix() {
>   if (Traits::Is64Bit) {
>     static constexpr uint8_t AddrSizeOverridePrefix = 0x67;
>     emitUint8(AddrSizeOverridePrefix);
>   }
> }

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:979: // possible to distinguish between the the "new"
low byte registers
On 2015/12/20 19:27:37, stichnot wrote:
> the the

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:982: static uint32_t Encoded_ah = 0x04;
On 2015/12/20 19:27:37, stichnot wrote:
> constexpr

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:984: isByteSizedType(Ty) && (Reg != Encoded_ah);
On 2015/12/20 19:27:37, stichnot wrote:
> Could you use something like Traits::RegisterSet::Encoded_ah instead?

Sure. I still needed the local Encoded_ah (which I renamed to Encoded_Reg_ah)
because Traits::RegisterSet::Encoded_Reg_ah's type might be different than
RegType.

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp
File src/IceInstX8664.cpp (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:111: // physical register (esp or ebp), and update the
Offset.
On 2015/12/20 19:27:37, stichnot wrote:
> Should that be rsp/rbp?

Not necessarily. Because x32, even though we manipulate 64-bit register
(r[es]p), We only ever use the lower 32 bits. This became "Stack or Frame
pointer".

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:124: if (getOffset() == 0 && Disp == 0) {
On 2015/12/20 19:27:37, stichnot wrote:
> Here and below, I think getOffset() should be compared against nullptr instead
> of 0.

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:219: (void)Target;
On 2015/12/20 19:27:37, stichnot wrote:
> remove this

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:965: //   mov ecx, ecx ==> reduntant, and dangerous in
x86-64. i64 zexting
On 2015/12/20 19:27:37, stichnot wrote:
> redundant

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:201: constexpr uint16_t Encoded_rAX = 0;
On 2015/12/20 19:27:37, stichnot wrote:
> It seems odd to capitalize AX here.
that's x86 nomenclature for a register than could be either eax or rax.

> 
> Also, don't you need to cast it to void for a NOASSERT/MINIMAL build?

https://codereview.chromium.org/1537703002/diff/220001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1513: // assert(DestReg ==
InstX86Base<Machine>::Traits::RegisterSet::Reg_edx);
On 2015/12/20 19:27:37, stichnot wrote:
> There should be *some* assert here, right?

Yes and no. These asserts should be in the constructor, not here (and in every
other instruction as well.) Having the assert in the ctor allows for easier
debugability -- i.e., you know where is the offending "emission" is. For now,
the assert is back for overall team morale happiness. :)

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:463: assert(static_cast<int32_t>(Canonical) ==
Traits::getBaseReg(Canonical));
On 2015/12/20 19:27:37, stichnot wrote:
> I think Canonical can/should be changed to int32_t (also in the x8632
version),
> and then the clumsy cast could go away.

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2849: assert(!T::Is64Bit);
On 2015/12/20 19:27:38, stichnot wrote:
> This assert seems completely unnecessary given the std::enable_if above.

Done.

https://codereview.chromium.org/1537703002/diff/220001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/1537703002/diff/220001/src/IceTypes.def#newco...
src/IceTypes.def:25: X(Target_X8664, "x86-64", false,  EM_X86_64,  0)           
                  \
On 2015/12/20 19:27:38, stichnot wrote:
> realign the columns

Done.

[John, 2015-12-21 13:44:52]

Description was changed from

==========
Subzero. x8664. Enables crosstests.

After a hiatus while x32 was not available in nacl's llvm, x8664 is
being revived.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

to

==========
Subzero. x8664. Resurrects the Target.

After a hiatus while x32 was not available in nacl's llvm, x8664 is
being revived. Rejoice!

The Target is now back to where it was before: the crosstests pass, and
SPEC2k builds and verifies.

Makefile.standalone still has the crosstests for x8664 disabled while
we wait for all the plumbing that's needed for x32 support on nacl's
toolchain to be available.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

[Jim Stichnoth, 2015-12-21 15:48:48]

1. "make -f Makefile.standalone DEBUG=1 CHECK_X86_ASM=1 check-unit" fails.  (All
the other "make presubmit" works.)

2. Can you make a preliminary native_client CL available so that I can try out
spec2k?

https://codereview.chromium.org/1537703002/diff/240001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1537703002/diff/240001/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:930: typename std::enable_if<!T::Is64Bit, void>::type
remove this

[John, 2015-12-21 21:30:51]

> 1. "make -f Makefile.standalone DEBUG=1 CHECK_X86_ASM=1 check-unit" fails. 
(All
> the other "make presubmit" works.)
It works now. :)

> 
> 2. Can you make a preliminary native_client CL available so that I can try out
> spec2k?
Not just a preliminary, these are all the changes needed to pnacl
infrastructure:
https://codereview.chromium.org/1541863002

> 
>
https://codereview.chromium.org/1537703002/diff/240001/src/IceAssemblerX86Base.h
> File src/IceAssemblerX86Base.h (right):
> 
>
https://codereview.chromium.org/1537703002/diff/240001/src/IceAssemblerX86Bas...
> src/IceAssemblerX86Base.h:930: typename std::enable_if<!T::Is64Bit,
void>::type
> remove this

Done.

[Jim Stichnoth, 2015-12-22 15:36:16]

LGTM.

We also need to deal with lit tests at some point... :)

https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone#ne...
Makefile.standalone:403: -e x8664,native,sse2 \
This doesn't actually do any x8664 tests, since no "-i" lines match x8664.

You might want
  -i x8664,native

Or, if you're trying to mirror x8632, change the first two "-e" to "-i" (and
then change the last one after sandboxing is implemented).

https://codereview.chromium.org/1537703002/diff/280001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1537703002/diff/280001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632Traits.h:584: }
I would "return -1;" to avoid compiler warnings/errors that, as far as I can
tell, are only being held back due to the compiler's recognition of
__attribute__((noreturn)) or __declspec(noreturn).

[John, 2015-12-22 16:13:04]

On 2015/12/22 15:36:16, stichnot wrote:
> LGTM.
> 
> We also need to deal with lit tests at some point... :)
Let me work on sandboxing first, than I'll add them.

> 
> https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone
> File Makefile.standalone (right):
> 
>
https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone#ne...
> Makefile.standalone:403: -e x8664,native,sse2 \
> This doesn't actually do any x8664 tests, since no "-i" lines match x8664.
> 
> You might want
>   -i x8664,native
> 
> Or, if you're trying to mirror x8632, change the first two "-e" to "-i" (and
> then change the last one after sandboxing is implemented).
> 
>
https://codereview.chromium.org/1537703002/diff/280001/src/IceTargetLoweringX...
> File src/IceTargetLoweringX8632Traits.h (right):
> 
>
https://codereview.chromium.org/1537703002/diff/280001/src/IceTargetLoweringX...
> src/IceTargetLoweringX8632Traits.h:584: }
> I would "return -1;" to avoid compiler warnings/errors that, as far as I can
> tell, are only being held back due to the compiler's recognition of
> __attribute__((noreturn)) or __declspec(noreturn).

[John, 2015-12-22 16:14:03]

Description was changed from

==========
Subzero. x8664. Resurrects the Target.

After a hiatus while x32 was not available in nacl's llvm, x8664 is
being revived. Rejoice!

The Target is now back to where it was before: the crosstests pass, and
SPEC2k builds and verifies.

Makefile.standalone still has the crosstests for x8664 disabled while
we wait for all the plumbing that's needed for x32 support on nacl's
toolchain to be available.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

to

==========
Subzero. x8664. Resurrects the Target.

After a hiatus while x32 was not available in nacl's llvm, x8664 is
being revived. Rejoice!

The Target is now back to where it was before: the crosstests pass, and
SPEC2k builds and verifies.

Makefile.standalone still has the crosstests for x8664 disabled while
we wait for all the plumbing that's needed for x32 support on nacl's
toolchain to be available.

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

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

[John, 2015-12-22 16:14:05]

Committed patchset #15 (id:280001) manually as
3c275ce1e832ba9ccfb730c4235db786cf080465 (presubmit successful).

[John, 2015-12-22 16:20:02]

https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone#ne...
Makefile.standalone:403: -e x8664,native,sse2 \
On 2015/12/22 15:36:16, stichnot wrote:
> This doesn't actually do any x8664 tests, since no "-i" lines match x8664.
> 
> You might want
>   -i x8664,native
> 
> Or, if you're trying to mirror x8632, change the first two "-e" to "-i" (and
> then change the last one after sandboxing is implemented).

I know. This is on purpose: if I enable them before pushing the nacl CL, then no
one will be able to make presubmit. :(

https://codereview.chromium.org/1537703002/diff/280001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1537703002/diff/280001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632Traits.h:584: }
On 2015/12/22 15:36:16, stichnot wrote:
> I would "return -1;" to avoid compiler warnings/errors that, as far as I can
> tell, are only being held back due to the compiler's recognition of
> __attribute__((noreturn)) or __declspec(noreturn).

but that's what that attribute is for... there's just something "weird" about
returning after reporting a fatal error. Besides, I want the compiler to yell if
it is unsure about report_fatal_error() terminating the program so that I can
add an exit(1) a the end of the function. In a way, the missing "return" carries
the semantic information that this function will not return at all.

[Jim Stichnoth, 2015-12-22 16:26:44]

https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1537703002/diff/280001/Makefile.standalone#ne...
Makefile.standalone:403: -e x8664,native,sse2 \
On 2015/12/22 16:20:01, John wrote:
> On 2015/12/22 15:36:16, stichnot wrote:
> > This doesn't actually do any x8664 tests, since no "-i" lines match x8664.
> > 
> > You might want
> >   -i x8664,native
> > 
> > Or, if you're trying to mirror x8632, change the first two "-e" to "-i" (and
> > then change the last one after sandboxing is implemented).
> 
> I know. This is on purpose: if I enable them before pushing the nacl CL, then
no
> one will be able to make presubmit. :(

Not true.  It's been common practice in Subzero to have to update to tip-of-tree
LLVM as necessary, particularly for the bitcode reader work.  And both options
worked for me when I tested.  Please get this enabled as soon possible, before
someone (me) breaks it again...