Subzero: Align the stack at the point of function calls.

Subzero: Align the stack at the point of function calls.

Be compatible with the x86-32 calling convention by ensuring that the
stack is aligned to 16 bytes at the point of the call
instruction. Also ensure that vector arguments passed on the stack are
16 byte aligned.

Also, make alloca instructions respect alignment.

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=105b704

[Jim Stichnoth, 2014-08-05 18:09:27]

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv.h:11: // convention
Period at the end of the sentence?

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv.h:18: #include "test_calling_conv.def"
I think you forgot to "git add" this file.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv.h:22: typedef int v4si32
__attribute__((vector_size(16)));
Consider #including vectors.h instead?

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
File crosstest/test_calling_conv_main.cpp (right):

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv_main.cpp:10: // Driver for cross testing the
compatibility of calling conventions
Period at end of sentence?

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv_main.cpp:18: #include <iostream>
Alphabetize the includes.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv_main.cpp:23: 
There should be a comment here describing the big picture of how this test
works.

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

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:134: uint32_t
getAdjustmentToAlignStackInBytes(uint32_t Size) {
Almost all the calls to this routine have a pattern like:

  foo += getAdjustmentToAlignStackInBytes(foo);

I think it might simplify things if you did it something like this:

  uint32_t applyStackAlignment(uint32_t Value) {
    assert((X86_STACK_ALIGNMENT_BYTES & (X86_STACK_ALIGNMENT_BYTES - 1) == 0);
// power of 2
    return (Value + X86_STACK_ALIGNMENT_BYTES - 1) & ~X86_STACK_ALIGNMENT_BYTES;
  }

  foo = applyStackAlignment(foo);

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:983: NeedsStackAlignment = true;
A couple things here.

1. It looks like alloca lowering is ignoring InstAlloca::getAlignInBytes().  As
long as you're dealing with alignment here, could you fix that? :)

2. It's not clear to me that the presence of an alloca instruction necessarily
requires 16-byte stack alignment.  Also, if e.g. an alloca with 16*K byte
alignment dominates all calls, then maybe NeedsStackAlignment could be reset to
false for the whole method.  Probably for now you can just add a comment that
we're taking the conservative approach for now.

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1002: _add(T, Ctx->getConstantInt(IceType_i32,
15));
Should these constants be in terms of X86_STACK_ALIGNMENT_BYTES?

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1642: // * Other arguments are placed on the
stack ordered according to the
This is unclear to me (and possibly wrong).  Does something like this seem
accurate and more precise?

Other arguments are pushed onto the stack in right-to-left order, such that the
left-most argument ends up on the top of the stack at the lowest memory address.

https://codereview.chromium.org/444443002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/alloc.ll (right):

https://codereview.chromium.org/444443002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/alloc.ll:53: ; CHECK-NEXT: and     eax,
18446744073709551600
What is a 64-bit integer immediate doing in an x86-32 program?

[wala, 2014-08-05 23:57:04]

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv.h:11: // convention
On 2014/08/05 18:09:26, stichnot wrote:
> Period at the end of the sentence?

Done.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv.h:18: #include "test_calling_conv.def"
On 2014/08/05 18:09:26, stichnot wrote:
> I think you forgot to "git add" this file.

Woops, done.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv.h:22: typedef int v4si32
__attribute__((vector_size(16)));
On 2014/08/05 18:09:26, stichnot wrote:
> Consider #including vectors.h instead?

Done.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
File crosstest/test_calling_conv_main.cpp (right):

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv_main.cpp:10: // Driver for cross testing the
compatibility of calling conventions
On 2014/08/05 18:09:26, stichnot wrote:
> Period at end of sentence?

Done.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv_main.cpp:18: #include <iostream>
On 2014/08/05 18:09:26, stichnot wrote:
> Alphabetize the includes.

Done.

https://codereview.chromium.org/444443002/diff/20001/crosstest/test_calling_c...
crosstest/test_calling_conv_main.cpp:23: 
On 2014/08/05 18:09:26, stichnot wrote:
> There should be a comment here describing the big picture of how this test
> works.

Done.

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

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:134: uint32_t
getAdjustmentToAlignStackInBytes(uint32_t Size) {
On 2014/08/05 18:09:27, stichnot wrote:
> Almost all the calls to this routine have a pattern like:
> 
>   foo += getAdjustmentToAlignStackInBytes(foo);
> 
> I think it might simplify things if you did it something like this:
> 
>   uint32_t applyStackAlignment(uint32_t Value) {
>     assert((X86_STACK_ALIGNMENT_BYTES & (X86_STACK_ALIGNMENT_BYTES - 1) == 0);
> // power of 2
>     return (Value + X86_STACK_ALIGNMENT_BYTES - 1) &
~X86_STACK_ALIGNMENT_BYTES;
>   }
> 
>   foo = applyStackAlignment(foo);
> 

Done.

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:983: NeedsStackAlignment = true;
On 2014/08/05 18:09:27, stichnot wrote:
> A couple things here.
> 
> 1. It looks like alloca lowering is ignoring InstAlloca::getAlignInBytes(). 
As
> long as you're dealing with alignment here, could you fix that? :)
> 
> 2. It's not clear to me that the presence of an alloca instruction necessarily
> requires 16-byte stack alignment.  Also, if e.g. an alloca with 16*K byte
> alignment dominates all calls, then maybe NeedsStackAlignment could be reset
to
> false for the whole method.  Probably for now you can just add a comment that
> we're taking the conservative approach for now.

Done.

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1002: _add(T, Ctx->getConstantInt(IceType_i32,
15));
On 2014/08/05 18:09:26, stichnot wrote:
> Should these constants be in terms of X86_STACK_ALIGNMENT_BYTES?

Done.

https://codereview.chromium.org/444443002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1642: // * Other arguments are placed on the
stack ordered according to the
On 2014/08/05 18:09:27, stichnot wrote:
> This is unclear to me (and possibly wrong).  Does something like this seem
> accurate and more precise?
> 
> Other arguments are pushed onto the stack in right-to-left order, such that
the
> left-most argument ends up on the top of the stack at the lowest memory
address.

Done.

https://codereview.chromium.org/444443002/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/alloc.ll (right):

https://codereview.chromium.org/444443002/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/alloc.ll:53: ; CHECK-NEXT: and     eax,
18446744073709551600
On 2014/08/05 18:09:27, stichnot wrote:
> What is a 64-bit integer immediate doing in an x86-32 program?

This should be -16. I'm not entirely sure what caused it to be emitted this way.
The current version of the code emits a 32 bit immediate.

[wala, 2014-08-06 03:07:30]

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

https://codereview.chromium.org/444443002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1021: if (AdjustAlignment) {
1018 - 1019 is better expressed as

add T, (Alignment - 1)
and T, -Alignment

Then the if can be eliminated.

I'll fix this tomorrow morning!

[Jim Stichnoth, 2014-08-06 18:26:16]

lgtm

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
File crosstest/test_calling_conv.cpp (right):

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
crosstest/test_calling_conv.cpp:21: #define CALL_AS_TYPE(Ty)
(reinterpret_cast<Ty *>(Callee))
Here and below, it would be nice to minimize the use of unbound variables in
macros.  I.e., change to:

  #define CALL_AS_TYPE(Ty, Callee) ...
  ...
  CALL_AS_TYPE(blah, Callee)(...);

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
crosstest/test_calling_conv.h:14: 
You removed the include guard - why not keep it?

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

https://codereview.chromium.org/444443002/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:124: const unsigned X86_MAX_XMM_ARGS = 4;
Generally, I've been preferring to use uint32_t rather than unsigned, in the
interest of clarity.  It would be good to make those changes here.  (I realize
I've missed that issue in some previous code reviews...)

https://codereview.chromium.org/444443002/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1000: bool AdjustAlignment = AlignmentParam >
X86_STACK_ALIGNMENT_BYTES;
It might be slightly clearer this way:

uint32_t Alignment = std::max(AlignmentParam, X86_STACK_ALIGNMENT_BYTES);
if (Alignment > X86_STACK_ALIGNMENT_BYTES) {

https://codereview.chromium.org/444443002/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1011: Value = (Value + Alignment - 1) &
-Alignment;
Should this instead do
  Value = applyStackAlignment(Value);
?

[wala, 2014-08-07 18:09:21]

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
File crosstest/test_calling_conv.cpp (right):

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
crosstest/test_calling_conv.cpp:21: #define CALL_AS_TYPE(Ty)
(reinterpret_cast<Ty *>(Callee))
On 2014/08/06 18:26:15, stichnot wrote:
> Here and below, it would be nice to minimize the use of unbound variables in
> macros.  I.e., change to:
> 
>   #define CALL_AS_TYPE(Ty, Callee) ...
>   ...
>   CALL_AS_TYPE(blah, Callee)(...);

Done.

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/444443002/diff/120001/crosstest/test_calling_...
crosstest/test_calling_conv.h:14: 
On 2014/08/06 18:26:16, stichnot wrote:
> You removed the include guard - why not keep it?

I changed test_calling_conv implementation changed from C to C++ - so I decided
to add the Subzero_ prefix in the C++ way by using a namespace. This requires
including the test_calling_conv.h header twice in the driver - which the include
guards won't allow.

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

https://codereview.chromium.org/444443002/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:124: const unsigned X86_MAX_XMM_ARGS = 4;
On 2014/08/06 18:26:16, stichnot wrote:
> Generally, I've been preferring to use uint32_t rather than unsigned, in the
> interest of clarity.  It would be good to make those changes here.  (I realize
> I've missed that issue in some previous code reviews...)

Done.

https://codereview.chromium.org/444443002/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1000: bool AdjustAlignment = AlignmentParam >
X86_STACK_ALIGNMENT_BYTES;
On 2014/08/06 18:26:16, stichnot wrote:
> It might be slightly clearer this way:
> 
> uint32_t Alignment = std::max(AlignmentParam, X86_STACK_ALIGNMENT_BYTES);
> if (Alignment > X86_STACK_ALIGNMENT_BYTES) {

Done.

https://codereview.chromium.org/444443002/diff/120001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1011: Value = (Value + Alignment - 1) &
-Alignment;
On 2014/08/06 18:26:16, stichnot wrote:
> Should this instead do
>   Value = applyStackAlignment(Value);
> ?

No. Alignment is a power of 2 at least as big as the stack alignment, but it
could be bigger.

[jvoung (off chromium), 2014-08-12 01:36:23]

LGTM

https://codereview.chromium.org/444443002/diff/140001/crosstest/test_calling_...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/444443002/diff/140001/crosstest/test_calling_...
crosstest/test_calling_conv.h:36: callee_vlvlvfvdviv_Ty callee_vlvlvfvdviv,
Subzero_callee_vlvlvfvdviv;
wow =)

Well, my only suggestion is maybe test having 12 bytes in between the vectors
(e.g., change the second int64_t into: "int64_t, int"). Or was there a reason
that you have "v4f32, int64_t, v4f32, int64_t"?

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

https://codereview.chromium.org/444443002/diff/140001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:990: // TODO(sehr,stichnot): align allocated
memory, minimize the number of
Is the "align allocated memory" part of the TODO now done? I guess there may be
more to do, in the sense that they may be over aligned with each one
independently aligning as if they need to preserve stack alignment.

[wala, 2014-08-12 02:56:09]

https://codereview.chromium.org/444443002/diff/140001/crosstest/test_calling_...
File crosstest/test_calling_conv.h (right):

https://codereview.chromium.org/444443002/diff/140001/crosstest/test_calling_...
crosstest/test_calling_conv.h:36: callee_vlvlvfvdviv_Ty callee_vlvlvfvdviv,
Subzero_callee_vlvlvfvdviv;
On 2014/08/12 01:36:23, jvoung wrote:
> wow =)
> 
> Well, my only suggestion is maybe test having 12 bytes in between the vectors
> (e.g., change the second int64_t into: "int64_t, int"). Or was there a reason
> that you have "v4f32, int64_t, v4f32, int64_t"?

Done. There wasn't really a systematic reason for all the parameter types.

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

https://codereview.chromium.org/444443002/diff/140001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:990: // TODO(sehr,stichnot): align allocated
memory, minimize the number of
On 2014/08/12 01:36:23, jvoung wrote:
> Is the "align allocated memory" part of the TODO now done? I guess there may
be
> more to do, in the sense that they may be over aligned with each one
> independently aligning as if they need to preserve stack alignment.

Good point, the part about aligning allocas is done.

What's left to do for full data alignment support (which the above comment does
not cover) is to align stack slots and to issue aligned loads and stores where
appropriate.

[wala, 2014-08-12 02:56:24]

Committed patchset #7 manually as 105b704 (presubmit successful).