Subzero. Buildable, non-functional TargetLoweringX8664.

Subzero. Buildable, non-functional TargetLoweringX8664.

This CL adds a TargetLoweringX8664 that inherits from TargetX86Base, but
other than that it does nothing to generate runnable code.

Things that need to be addressed in follow up CLs:
1) lowerCall
2) lowerArguments
3) lowerRet
4) addPrologue
5) addEpilogue
6) Native 64-bit arithmetic
7) 32- to 64-bit addressing

(7) will be particularly interesting. Pointers in Pexes are always
32-bit wide, so pexes have a de facto 32-bit address space. In
Sandboxed mode that's solved by using RZP (i.e., r15) as a base
register. For native codegen, we still need to decide what to do
-- very likely we will start targeting X32.

NOTE: This CL also

s/IceType_ForceRexW/RexTypeForceRexW/g

because I forgot to do it in the X8664 assembler cl.

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=453660ff4f0702f5cd95acdaf9e88196341b9cc6

[John, 2015-07-29 17:43:16]

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

[John, 2015-07-29 17:43:16]

https://codereview.chromium.org/1257643004/diff/20001/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1257643004/diff/20001/src/IceAssemblerX86Base...
src/IceAssemblerX86Base.h:895: static constexpr Type RexTypeForceRexW =
IceType_i64;
Sorry for piggybacking this change here...

https://codereview.chromium.org/1257643004/diff/20001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1257643004/diff/20001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:2783: template <typename T = typename
InstX86Base<Machine>::Traits>
These are not strictly needed, but adding them prevents X8664 from having these
type aliases -- which use disabled (in x8664) assembler instructions.

[Jim Stichnoth, 2015-07-30 19:11:08]

lgtm

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:52: const
MachineTraits<TargetX8664>::TableIcmp64Type
Presumably this is just cargo-culting that will later go away?  x86-64 lowering
should (counter-intuitively) need only the C_32 column and not the C?_64
columns.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:79: const uint32_t
MachineTraits<TargetX8664>::X86_STACK_ALIGNMENT_BYTES = 16;
Can/should this be constexpr?

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

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:613: bool Randomized;
This should be "bool Randomized = false;" and then dropped from the ctor
initializer list.

[jvoung (off chromium), 2015-07-30 21:16:22]

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:51: private:
This "private:" is already covered by the (new) previous one?

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:227: Writer->writeDataSection(Vars,
llvm::ELF::R_386_32, SectionSuffix);
TODO(jpp) to change this reloc to be not 386.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.h (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.h:47: Constant *TPOffset =
Ctx->getConstantInt8(0x20);
Hmm this magic offset doesn't seem particularly familiar. It's not just the
return value of calling nacl_read_tp? Then the bitcode decides on which offset
to use from there. The sandboxing of that pointer (+RZP) happens separately as
part of the load sandboxing, I thought.

E.g.,

  %tls_raw = call i8* @llvm.nacl.read.tp()
  %tls_raw.asint = ptrtoint i8* %tls_raw to i32
  %gep = add i32 %tls_raw.asint, -1184
  %gep.asptr = inttoptr i32 %gep to i32*
  %0 = load i32, i32* %gep.asptr, align 1, !dbg !9330
  %cmp = icmp ne i32 %0, 1, !dbg !9332
  %gep37 = add i32 %tls_raw.asint, -1168
  %gep37.asptr = inttoptr i32 %gep37 to i32*
  %1 = load i32, i32* %gep37.asptr, align 1

For slots at -1184 and -1168.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.h:59: Variable *RZP = makeReg(IceType_i32,
Traits::RegisterSet::Reg_r15d);
Should this use getPhysicalRegister() and RZP marked live in on function entry?
Also, RZP should be const throughout the function and not get allocated.

[John, 2015-07-31 21:05:54]

as per lgtm, submitting.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:51: private:
On 2015/07/30 21:16:21, jvoung wrote:
> This "private:" is already covered by the (new) previous one?

Done.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:52: const
MachineTraits<TargetX8664>::TableIcmp64Type
On 2015/07/30 19:11:08, stichnot wrote:
> Presumably this is just cargo-culting that will later go away?  x86-64
lowering
> should (counter-intuitively) need only the C_32 column and not the C?_64
> columns.

Correct. As the CL description states I did not implement native i64 support. It
is not my intention to have a working x86-64 backend: I just want the pieces of
code that need to be specialized for 32 v. 64 bit pulled out of the template
class so that I can later change what needs to be changed.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:79: const uint32_t
MachineTraits<TargetX8664>::X86_STACK_ALIGNMENT_BYTES = 16;
On 2015/07/30 19:11:08, stichnot wrote:
> Can/should this be constexpr?

No, it can not. having this as a constexpr causes link-time errors. I added a
comment to the traits class a while ago why this is the case:

Stack alignment. This is defined in IceTargetLoweringX8632.cpp because it is
used as an argument to std::max(), and the default std::less<T> has an
operator(T const&, T const&) which requires this member to have an address.

Well, I guess the comment is pretty awful (it should be OK to pass a constant as
a T const&), but in this case (maybe something to do with templates) having a
constexpr constant does not work.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:227: Writer->writeDataSection(Vars,
llvm::ELF::R_386_32, SectionSuffix);
On 2015/07/30 21:16:22, jvoung wrote:
> TODO(jpp) to change this reloc to be not 386.

Done.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.h (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.h:47: Constant *TPOffset =
Ctx->getConstantInt8(0x20);
On 2015/07/30 21:16:22, jvoung wrote:
> Hmm this magic offset doesn't seem particularly familiar. It's not just the
> return value of calling nacl_read_tp? Then the bitcode decides on which offset
> to use from there. The sandboxing of that pointer (+RZP) happens separately as
> part of the load sandboxing, I thought.
> 
> E.g.,
> 
>   %tls_raw = call i8* @llvm.nacl.read.tp()
>   %tls_raw.asint = ptrtoint i8* %tls_raw to i32
>   %gep = add i32 %tls_raw.asint, -1184
>   %gep.asptr = inttoptr i32 %gep to i32*
>   %0 = load i32, i32* %gep.asptr, align 1, !dbg !9330
>   %cmp = icmp ne i32 %0, 1, !dbg !9332
>   %gep37 = add i32 %tls_raw.asint, -1168
>   %gep37.asptr = inttoptr i32 %gep37 to i32*
>   %1 = load i32, i32* %gep37.asptr, align 1
> 
> For slots at -1184 and -1168.

I got confused with the comments in

native_client/src/untrusted/nacl/tls_params.h

Done.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.h:59: Variable *RZP = makeReg(IceType_i32,
Traits::RegisterSet::Reg_r15d);
On 2015/07/30 21:16:22, jvoung wrote:
> Should this use getPhysicalRegister() and RZP marked live in on function
entry?
> Also, RZP should be const throughout the function and not get allocated.

N/A anymore (this method was removed.)

As to RZP being constant, this is not a problem -- yet. In these CLs (there 4 or
5) I am not trying to have a correct x86-64 backend -- I am just moving code
from the 32 to the 64 bit backend, with some minimal changes. The bulk of the
(interesting) work (i.e., getting this code to run) is coming after I have all
things in place and can actually start testing the code it generates. What I
want to say is -- don't overthink about whether something is horribly wrong. It
probably is and will be fixed soon in a separate CL.

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

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664Traits.h:613: bool Randomized;
On 2015/07/30 19:11:08, stichnot wrote:
> This should be "bool Randomized = false;" and then dropped from the ctor
> initializer list.

Done.

[jvoung (off chromium), 2015-07-31 21:21:05]

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.h (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.h:59: Variable *RZP = makeReg(IceType_i32,
Traits::RegisterSet::Reg_r15d);
On 2015/07/31 21:05:54, John wrote:
> On 2015/07/30 21:16:22, jvoung wrote:
> > Should this use getPhysicalRegister() and RZP marked live in on function
> entry?
> > Also, RZP should be const throughout the function and not get allocated.
> 
> N/A anymore (this method was removed.)
> 
> As to RZP being constant, this is not a problem -- yet. In these CLs (there 4
or
> 5) I am not trying to have a correct x86-64 backend -- I am just moving code
> from the 32 to the 64 bit backend, with some minimal changes. The bulk of the
> (interesting) work (i.e., getting this code to run) is coming after I have all
> things in place and can actually start testing the code it generates. What I
> want to say is -- don't overthink about whether something is horribly wrong.
It
> probably is and will be fixed soon in a separate CL.

Okay sounds fine for this CL. Just raising awareness, in case it helps with
testing later =)

[John, 2015-07-31 21:29:07]

non-trivial, but easy, merge later, I'll land this as soon as I finish compiling
spec and running tests.

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.h (right):

https://codereview.chromium.org/1257643004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.h:59: Variable *RZP = makeReg(IceType_i32,
Traits::RegisterSet::Reg_r15d);
On 2015/07/31 21:21:05, jvoung wrote:
> On 2015/07/31 21:05:54, John wrote:
> > On 2015/07/30 21:16:22, jvoung wrote:
> > > Should this use getPhysicalRegister() and RZP marked live in on function
> > entry?
> > > Also, RZP should be const throughout the function and not get allocated.
> > 
> > N/A anymore (this method was removed.)
> > 
> > As to RZP being constant, this is not a problem -- yet. In these CLs (there
4
> or
> > 5) I am not trying to have a correct x86-64 backend -- I am just moving code
> > from the 32 to the 64 bit backend, with some minimal changes. The bulk of
the
> > (interesting) work (i.e., getting this code to run) is coming after I have
all
> > things in place and can actually start testing the code it generates. What I
> > want to say is -- don't overthink about whether something is horribly wrong.
> It
> > probably is and will be fixed soon in a separate CL.
> 
> Okay sounds fine for this CL. Just raising awareness, in case it helps with
> testing later =)

You are  awesome at finding things that will be painful to fix later for they
are small (but with huge implications.) I was just trying to take some of the
load off your back as these CLs are really meant to lay the "canvas" for the
actual x86-64 implementation. I am sorry if I came across as harsh or impolite.

[John, 2015-07-31 21:52:56]

Committed patchset #7 (id:120001) manually as
453660ff4f0702f5cd95acdaf9e88196341b9cc6 (presubmit successful).