Add support for passing and returning vectors in accordance with the x86 calling convention.

Add support for passing and returning vectors in accordance with the x86 calling convention.

- Add TargetLowering::lowerArguments() as a new stage in TargetLowering.
- Add support for passing arguments/return values in XMM registers in the x86 target.

BUG=none
R=jvoung@chromium.org, stichnot@chromium.org

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

[Jim Stichnoth, 2014-07-08 18:03:15]

https://codereview.chromium.org/372113005/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/1/src/IceInstX8632.cpp#newcode675
src/IceInstX8632.cpp:675: Str << "storeq." << getSrc(0)->getType() << " ";
storeq?  should it be storep?

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:415: // Classify each argument as stack or
register.  Add a hint to the
As we discussed in person, I think a simpler approach for register arguments is
to replace them in the function argument list with alternate versions with
specific register assignments, and then add explicit copies after the prolog and
before the first instruction of the entry block.  Then none of the extra
bookkeeping in class Variable is needed.  E.g.:

  int f(int i, vector v1, vector v2) { ... }

is transformed to something like this:

  int f(int i, vector reg_xmm0_v1, vector reg_xmm1_v2) {
    v1 = reg_xmm0_v1;
    v2 = reg_xmm1_v2;
    ...
  }

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1352: _sub(esp, Ctx->getConstantInt(IceType_i8,
16));
Use a symbolic constant instead of 16.  (And reuse an existing symbolic constant
if possible.)

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1368: // get dead code eliminated.
Mention in the comment that the dead code elimination comes as a result of the
FakeKill instruction inserted below.

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1378: Variable *xmm0 = NULL;
Can you rename variable eax (above) to something like ReturnReg, and use that
instead of this xmm0 variable?  The edx variable would also need to be renamed
appropriately/consistently.

[wala, 2014-07-08 22:38:58]

https://codereview.chromium.org/372113005/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/1/src/IceInstX8632.cpp#newcode675
src/IceInstX8632.cpp:675: Str << "storeq." << getSrc(0)->getType() << " ";
On 2014/07/08 18:03:15, stichnot wrote:
> storeq?  should it be storep?

Done.

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:415: // Classify each argument as stack or
register.  Add a hint to the
On 2014/07/08 18:03:15, stichnot wrote:
> As we discussed in person, I think a simpler approach for register arguments
is
> to replace them in the function argument list with alternate versions with
> specific register assignments, and then add explicit copies after the prolog
and
> before the first instruction of the entry block.  Then none of the extra
> bookkeeping in class Variable is needed.  E.g.:
> 
>   int f(int i, vector v1, vector v2) { ... }
> 
> is transformed to something like this:
> 
>   int f(int i, vector reg_xmm0_v1, vector reg_xmm1_v2) {
>     v1 = reg_xmm0_v1;
>     v2 = reg_xmm1_v2;
>     ...
>   }

Done.

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1352: _sub(esp, Ctx->getConstantInt(IceType_i8,
16));
On 2014/07/08 18:03:15, stichnot wrote:
> Use a symbolic constant instead of 16.  (And reuse an existing symbolic
constant
> if possible.)

Done, but by using the argument width directly without a constant.

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1368: // get dead code eliminated.
On 2014/07/08 18:03:15, stichnot wrote:
> Mention in the comment that the dead code elimination comes as a result of the
> FakeKill instruction inserted below.

Done.

https://codereview.chromium.org/372113005/diff/1/src/IceTargetLoweringX8632.c...
src/IceTargetLoweringX8632.cpp:1378: Variable *xmm0 = NULL;
On 2014/07/08 18:03:15, stichnot wrote:
> Can you rename variable eax (above) to something like ReturnReg, and use that
> instead of this xmm0 variable?  The edx variable would also need to be renamed
> appropriately/consistently.

Done.

[jvoung (off chromium), 2014-07-09 04:26:55]

not much to add, but a few comments below

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:436: for (SizeT I = 0, E = Args.size(); I < E &&
NumXmmArgs < 4; ++I) {
Might be worth making the "4" a symbolic constant too. Then the file where the
constant is defined can also document the calling convention. At least for LLVM
it's kind of nice to have a file like "X86CallingConv.td" that also serves as a
reference. On the other hand, we only support one calling convention, so it's
not that big of a reference =)

https://codereview.chromium.org/372113005/diff/60001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-arg.ll (right):

https://codereview.chromium.org/372113005/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arg.ll:164: ; CHECK: movups  xmm0, xmmword ptr
[esp+4]
What are the first few "CHECK: movups xmm0, xmmword ptr [esp+4]", etc. related
to? Setting up the xmm0-3 "actual" arguments for the call?

Or is there some spill code earlier on, that puts arg9 at esp+4, arg8 at esp+20,
etc.? Otherwise, shouldn't arg9 (start) further up the stack than arg0 from the
caller's perspective?

Similarly, are "ARG5" == %arg4, and "ARG6" == "%arg5" ?

https://codereview.chromium.org/372113005/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arg.ll:170: ; CHECK: sub esp, 16
At some point we might want to consolidate the stack adjustments for function
calls.

It's especially bad for NaCl x86-64 and ARM, since each time you add or subtract
rsp/esp, you have to restore the invariant that the stack pointer is within
bounds (zero-extend + add r15 or bic). E.g., naclasp and naclssp:
https://developer.chrome.com/native-client/reference/sandbox_internals/x86-64...

Maybe at some point the caller's prologue would just reserve enough space for
any of the calls, once.

[Jim Stichnoth, 2014-07-09 16:56:13]

LGTM with minor nits.

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:436: for (SizeT I = 0, E = Args.size(); I < E &&
NumXmmArgs < 4; ++I) {
On 2014/07/09 04:26:53, jvoung wrote:
> Might be worth making the "4" a symbolic constant too. Then the file where the
> constant is defined can also document the calling convention. At least for
LLVM
> it's kind of nice to have a file like "X86CallingConv.td" that also serves as
a
> reference. On the other hand, we only support one calling convention, so it's
> not that big of a reference =)

I agree, except that between x86-32 and x86-64 lowering (not to mention raw
versus sandboxed), there will likely be significant refactoring/reuse that makes
this more relevant.

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1384: // registers after the call.
I think it wouldn't hurt to assert(RegisterArgs.size()==NumXmmArgs) here.

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1459: } else if (ReturnReg) {
Don't use "else if" here.
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

[wala, 2014-07-09 19:05:13]

I discovered two issues with the way that arguments to call instructions were
lowered:
1) the stack adjustment wasn't getting updates for vector arguments
2) vector arguments were being lowered in the wrong order

The current changes should fix those issues. I have changed the tests which also
were wrong on the above points. The updated tests should have the correct stack
offsets.

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:436: for (SizeT I = 0, E = Args.size(); I < E &&
NumXmmArgs < 4; ++I) {
On 2014/07/09 16:56:13, stichnot wrote:
> On 2014/07/09 04:26:53, jvoung wrote:
> > Might be worth making the "4" a symbolic constant too. Then the file where
the
> > constant is defined can also document the calling convention. At least for
> LLVM
> > it's kind of nice to have a file like "X86CallingConv.td" that also serves
as
> a
> > reference. On the other hand, we only support one calling convention, so
it's
> > not that big of a reference =)
> 
> I agree, except that between x86-32 and x86-64 lowering (not to mention raw
> versus sandboxed), there will likely be significant refactoring/reuse that
makes
> this more relevant.

Done.

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1384: // registers after the call.
On 2014/07/09 16:56:13, stichnot wrote:
> I think it wouldn't hurt to assert(RegisterArgs.size()==NumXmmArgs) here.

Unnecessary in the revised code.

https://codereview.chromium.org/372113005/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1459: } else if (ReturnReg) {
On 2014/07/09 16:56:13, stichnot wrote:
> Don't use "else if" here.
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Done.

https://codereview.chromium.org/372113005/diff/60001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/vector-arg.ll (right):

https://codereview.chromium.org/372113005/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arg.ll:164: ; CHECK: movups  xmm0, xmmword ptr
[esp+4]
On 2014/07/09 04:26:53, jvoung wrote:
> What are the first few "CHECK: movups xmm0, xmmword ptr [esp+4]", etc. related
> to? Setting up the xmm0-3 "actual" arguments for the call?
> 
> Or is there some spill code earlier on, that puts arg9 at esp+4, arg8 at
esp+20,
> etc.? Otherwise, shouldn't arg9 (start) further up the stack than arg0 from
the
> caller's perspective?
> 
> Similarly, are "ARG5" == %arg4, and "ARG6" == "%arg5" ?

The order that the vector arguments are passed is wrong. It should be reversed.
This test was also wrong. Good catch, thank you.

https://codereview.chromium.org/372113005/diff/60001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/vector-arg.ll:170: ; CHECK: sub esp, 16
On 2014/07/09 04:26:53, jvoung wrote:
> At some point we might want to consolidate the stack adjustments for function
> calls.
> 
> It's especially bad for NaCl x86-64 and ARM, since each time you add or
subtract
> rsp/esp, you have to restore the invariant that the stack pointer is within
> bounds (zero-extend + add r15 or bic). E.g., naclasp and naclssp:
>
https://developer.chrome.com/native-client/reference/sandbox_internals/x86-64...
> 
> Maybe at some point the caller's prologue would just reserve enough space for
> any of the calls, once.

Added a note in the test and in lowerCall().

[jvoung (off chromium), 2014-07-09 20:49:00]

Looks pretty good to me.

https://codereview.chromium.org/372113005/diff/120001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:617: if (isVectorType(Arg->getType()) &&
NumXmmArgs < 4) {
4 -> X86_MAX_XMM_ARGS here too?

https://codereview.chromium.org/372113005/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1308: const bool AllowOverlap = false;
I haven't really thought about this much, but why did this change?

An issue exposed by the lowerArguments() assignments?

https://codereview.chromium.org/372113005/diff/120001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/vector-arg.ll (right):

https://codereview.chromium.org/372113005/diff/120001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-arg.ll:177: ; CHECK: movups  xmm0, xmmword ptr
[esp+116]
If it's easy, it might be worth doing this first (before the various pushes and
sub esp), so that the mem operand displacements are more likely to be small (and
could then be encoded by the assembler as 1 byte... maybe).

[wala, 2014-07-09 21:55:34]

https://codereview.chromium.org/372113005/diff/120001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/372113005/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:617: if (isVectorType(Arg->getType()) &&
NumXmmArgs < 4) {
On 2014/07/09 20:49:00, jvoung wrote:
> 4 -> X86_MAX_XMM_ARGS here too?

Done.

https://codereview.chromium.org/372113005/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1308: const bool AllowOverlap = false;
On 2014/07/09 20:49:00, jvoung wrote:
> I haven't really thought about this much, but why did this change?
> 
> An issue exposed by the lowerArguments() assignments?

This line is a relic of my personal experiments that carelessly made it into the
CL. Reverted to AllowOverlap = true.

https://codereview.chromium.org/372113005/diff/120001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/vector-arg.ll (right):

https://codereview.chromium.org/372113005/diff/120001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-arg.ll:177: ; CHECK: movups  xmm0, xmmword ptr
[esp+116]
On 2014/07/09 20:49:00, jvoung wrote:
> If it's easy, it might be worth doing this first (before the various pushes
and
> sub esp), so that the mem operand displacements are more likely to be small
(and
> could then be encoded by the assembler as 1 byte... maybe).

The original motivation I had for lowering arguments passed in registers after
lowering the stack arguments was to reduce register pressure.

I think that the advantages and disadvantages to both approaches need to be
evaluated when it comes time to tune the code generator.  I will note the
motivations for the different strategies in the comments but I will not change
the current algorithm.

[jvoung (off chromium), 2014-07-09 22:36:55]

LGTM

https://codereview.chromium.org/372113005/diff/120001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/vector-arg.ll (right):

https://codereview.chromium.org/372113005/diff/120001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/vector-arg.ll:177: ; CHECK: movups  xmm0, xmmword ptr
[esp+116]
On 2014/07/09 21:55:34, wala wrote:
> On 2014/07/09 20:49:00, jvoung wrote:
> > If it's easy, it might be worth doing this first (before the various pushes
> and
> > sub esp), so that the mem operand displacements are more likely to be small
> (and
> > could then be encoded by the assembler as 1 byte... maybe).
> 
> The original motivation I had for lowering arguments passed in registers after
> lowering the stack arguments was to reduce register pressure.
> 
> I think that the advantages and disadvantages to both approaches need to be
> evaluated when it comes time to tune the code generator.  I will note the
> motivations for the different strategies in the comments but I will not change
> the current algorithm. 

Ah, right, register pressure -- tuning later sounds good.

[wala, 2014-07-09 23:33:26]

Committed patchset #6 manually as r45a0623 (presubmit successful).