Subzero. Implements x86-64 lowerCall.

Subzero. Implements x86-64 lowerCall.

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

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

[John, 2015-07-31 22:24:54]

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

[John, 2015-07-31 22:24:55]

Yet another one of those ground work CLs...

[jvoung (off chromium), 2015-07-31 23:53:49]

https://codereview.chromium.org/1266673003/diff/40001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/1266673003/diff/40001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:1: //===- subzero/src/IceInstX8664.h - x86-64 machine
instructions -*- C++ -*-===//
Is the codereview page accurate? Looks like this is still the src/IceInstX8632.h
file and not x86-64.

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

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:121: "Mismtach between MAX_GPR_ARGS and
GprForArgNum.");
"Mismatch"

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:183: ParameterAreaSizeBytes +=
typeWidthInBytesOnStack(Arg->getType());
Is typeWidthInBytesOnStack defined differently for x86-32 vs x86-64 (8 bytes for
32-bit operand)?

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:241: case IceType_void:
This is the same in the other targets, but it seems like it should have been an
error to have a Dest w/ a void type? Do you remember anything about this Jim?

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:310: if (!ReturnReg)
Seems a bit odd to have a Dest, but no ReturnReg.

[John, 2015-08-01 15:49:55]

https://codereview.chromium.org/1266673003/diff/40001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/1266673003/diff/40001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:1: //===- subzero/src/IceInstX8664.h - x86-64 machine
instructions -*- C++ -*-===//
On 2015/07/31 23:53:48, jvoung wrote:
> Is the codereview page accurate? Looks like this is still the
src/IceInstX8632.h
> file and not x86-64.

I failed to git commit && git cl upload. Done.

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

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:121: "Mismtach between MAX_GPR_ARGS and
GprForArgNum.");
On 2015/07/31 23:53:49, jvoung wrote:
> "Mismatch"

"Doen." :)

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:183: ParameterAreaSizeBytes +=
typeWidthInBytesOnStack(Arg->getType());
On 2015/07/31 23:53:49, jvoung wrote:
> Is typeWidthInBytesOnStack defined differently for x86-32 vs x86-64 (8 bytes
for
> 32-bit operand)?

We could avoid the overhead, but if we want to be compliant with the x86-64 abi,
all arguments are at least 64-bit wide. I could not find any information about
x32 (i.e., x86-32 ILP32), so I am assuming they abide by the same calling
convention.

Disclaimer: it is **very** likely that this method will be moved back to the
template class. I believe that, with the right set of target traits this method
is fully templatizeable.

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:310: if (!ReturnReg)
On 2015/07/31 23:53:49, jvoung wrote:
> Seems a bit odd to have a Dest, but no ReturnReg.

Yes, indeed. This became an assert(ReturnReg);

[jvoung (off chromium), 2015-08-03 17:04:55]

LGTM

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

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:152: using OperandList = std::vector<Operand *>;
Might be able to use "llvm::SmallVector<Operand *, 8>", assuming there are few
operands (and Xmm have a limit of Traits::X86_MAX_XMM_ARGS, etc.).

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:183: ParameterAreaSizeBytes +=
typeWidthInBytesOnStack(Arg->getType());
On 2015/08/01 15:49:55, John wrote:
> On 2015/07/31 23:53:49, jvoung wrote:
> > Is typeWidthInBytesOnStack defined differently for x86-32 vs x86-64 (8 bytes
> for
> > 32-bit operand)?
> 
> We could avoid the overhead, but if we want to be compliant with the x86-64
abi,
> all arguments are at least 64-bit wide. I could not find any information about
> x32 (i.e., x86-32 ILP32), so I am assuming they abide by the same calling
> convention.

Yes, my understanding is that the calling convention is like x86-64 except the
sizing of things. So via-stack arguments are still as if you did a push and
should be rounded up to 8 bytes at a time.

Some docs here for x32: https://sites.google.com/site/x32abi/documents

Right now it looks like Subzero is using 4 bytes for both x86-32 and x86-64
though?


> 
> Disclaimer: it is **very** likely that this method will be moved back to the
> template class. I believe that, with the right set of target traits this
method
> is fully templatizeable.


Okay cool.

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:310: if (!ReturnReg)
On 2015/08/01 15:49:55, John wrote:
> On 2015/07/31 23:53:49, jvoung wrote:
> > Seems a bit odd to have a Dest, but no ReturnReg.
> 
> Yes, indeed. This became an assert(ReturnReg);

New patch upload?

[Jim Stichnoth, 2015-08-05 04:43:52]

lgtm

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

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:241: case IceType_void:
On 2015/07/31 23:53:49, jvoung wrote:
> This is the same in the other targets, but it seems like it should have been
an
> error to have a Dest w/ a void type? Do you remember anything about this Jim?

Yeah, I don't remember why it ended up this way, but if Dest is non-nullptr then
its type should definitely not be void.

Also, llvm_unreachable is OK as a placeholder while building out the code, but
use llvm::report_fatal_error for stable code.

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

https://codereview.chromium.org/1266673003/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:124: }
// end of anonymous namespace

[John, 2015-08-05 16:53:03]

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

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:152: using OperandList = std::vector<Operand *>;
On 2015/08/03 17:04:55, jvoung wrote:
> Might be able to use "llvm::SmallVector<Operand *, 8>", assuming there are few
> operands (and Xmm have a limit of Traits::X86_MAX_XMM_ARGS, etc.).

Done.

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:183: ParameterAreaSizeBytes +=
typeWidthInBytesOnStack(Arg->getType());
Oh, now I see what you mean. :)
I added a TODO in typeWidthInBytesOnStack.

https://codereview.chromium.org/1266673003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:310: if (!ReturnReg)
On 2015/08/03 17:04:55, jvoung wrote:
> On 2015/08/01 15:49:55, John wrote:
> > On 2015/07/31 23:53:49, jvoung wrote:
> > > Seems a bit odd to have a Dest, but no ReturnReg.
> > 
> > Yes, indeed. This became an assert(ReturnReg);
> 
> New patch upload?

Done.

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

https://codereview.chromium.org/1266673003/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:124: }
On 2015/08/05 04:43:52, stichnot wrote:
> // end of anonymous namespace

Done.

[John, 2015-08-05 17:14:07]

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