Introduce StackArgumentsAccessor class for X64

Introduce StackArgumentsAccessor class for X64

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16345

[haitao.feng, 2013-07-30 12:19:27]

Migrated from https://codereview.chromium.org/19857006/ and rebased with master.

[haitao.feng, 2013-07-31 00:24:50]

I choose the name StackOperandForArgument as both the "rsp + disp" and "rsp +
index * scale + disp" in this CL are used to access argument. Receiver is
considered as argument 0. For non-argument access, I used kPCOnStackSize instead
of kPointerSize and explained them in place.

https://codereview.chromium.org/21123008/diff/1/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/builtins-x64.cc#newco...
src/x64/builtins-x64.cc:711: __ SmiToInteger32(r10, Operand(rsp,
kPCOnStackSize));
Use kPCOnStackSize instead of kPointerSize.

https://codereview.chromium.org/21123008/diff/1/src/x64/builtins-x64.cc#newco...
src/x64/builtins-x64.cc:720: __ movq(rax, Operand(rsp, kPCOnStackSize +
kPointerSize));
Use kPCOnStackSize instead of kPointerSize.

https://codereview.chromium.org/21123008/diff/1/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/stub-cache-x64.cc#new...
src/x64/stub-cache-x64.cc:1077: __ movq(Operand(rsp, kPCOnStackSize),
object_reg);
Use kPCOnStackSize instead of kPointerSize.

https://codereview.chromium.org/21123008/diff/1/src/x64/stub-cache-x64.cc#new...
src/x64/stub-cache-x64.cc:1137: __ movq(Operand(rsp, kPCOnStackSize), reg);
Use kPCOnStackSize instead of kPointerSize.

[danno, 2013-07-31 07:48:37]

https://codereview.chromium.org/21123008/diff/1/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/builtins-x64.cc#newco...
src/x64/builtins-x64.cc:841: __ movq(rdi, StackOperandForArgument(rax,
times_pointer_size,
Does this ever no use "time_pointer_size"? If so, you can reduce the number of
arguments to StackOperandForArgument to just a single Register. You can also
make the disp parameter a parameter with default 0.

https://codereview.chromium.org/21123008/diff/1/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/ic-x64.cc#newcode968
src/x64/ic-x64.cc:968: // Get the receiver of the function from the stack; 1 ~
return address.
I don't understand the comment, what do you mean by "1 ~ return address"? Just
use StackOperandForReceiver as described below, then I don't think you need a
comment at all.

https://codereview.chromium.org/21123008/diff/1/src/x64/ic-x64.cc#newcode1306
src/x64/ic-x64.cc:1306: __ movq(rdx, StackOperandForArgument((argc + 1) *
kPointerSize));
Just to make it even prettier/more readible, how about introducing a
StackOperandForReceiver where you just pass argc, and it calls
StackOperandForArgument with (argc+1)?

https://codereview.chromium.org/21123008/diff/1/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/macro-assembler-x64.h...
src/x64/macro-assembler-x64.h:1525: return Operand(rsp, disp + kPCOnStackSize -
kPointerSize);
Does it make sense to use an index rather than displacement? It would simplify
the usage sites. i.e.:

return Operand(rsp, (index * kPointerSize) + kPCOnStackSize - kPointerSize +
extra_disp);

extra_disp can be a default parameter with value 0

https://codereview.chromium.org/21123008/diff/1/src/x64/macro-assembler-x64.h...
src/x64/macro-assembler-x64.h:1533: int32_t disp) {
Same here (index rather than displacement)?

https://codereview.chromium.org/21123008/diff/1/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/stub-cache-x64.cc#new...
src/x64/stub-cache-x64.cc:2007: __ movq(index, StackOperandForArgument((argc -
0) * kPointerSize));
just argc, I don't think the "- 0" makes anything clearer here

https://codereview.chromium.org/21123008/diff/1/src/x64/stub-cache-x64.cc#new...
src/x64/stub-cache-x64.cc:2088: __ movq(index, StackOperandForArgument((argc -
0) * kPointerSize));
just argc, I don't think the "- 0" makes anything clearer here

[haitao.feng, 2013-07-31 09:25:20]

https://codereview.chromium.org/21123008/diff/1/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/21123008/diff/1/src/x64/macro-assembler-x64.h...
src/x64/macro-assembler-x64.h:1525: return Operand(rsp, disp + kPCOnStackSize -
kPointerSize);
I have thought about this. I did not use it as index actually is counted from
stack pointer, not from argument0. The reader might think
StackOperandForArgument(index) is accessing the ith argument.

How about defining this function as:
inline Operand StackOperandForReversedArgument(int index) {
  return Operand(rsp, (index * kPointerSize) + kPCOnStackSize);
}
Index is counted from the argument n (rsp + kPCOnStackSize), instead of argument
0 or rsp. If we want it from argument 0, we might need two arguments (one index,
one argc). 

inline Operand StackOperandForReceiver(int argc) {
  return StackOperandForReversedArgument(argc);
}

If you are OK with this, I will start updating the codes to use this interface.

[danno, 2013-07-31 09:30:55]

Yes, this sounds fine.

However, I still think in this case you will have to have:

inline Operand StackOperandForReceiver(int argc) {
  return StackOperandForReversedArgument(argc + 1);
}

[haitao.feng, 2013-07-31 13:15:57]

Danno, thanks for the review. All comments are addressed. Please take another
look.

On 2013/07/31 09:30:55, danno wrote:
> Yes, this sounds fine.
> 
> However, I still think in this case you will have to have:
> 
If I understand the codes right, the receiver is at argc offset of the last
argument. The 1 is for return address in the original code. 
> inline Operand StackOperandForReceiver(int argc) {
>   return StackOperandForReversedArgument(argc + 1);
> }

[danno, 2013-08-02 08:48:00]

Thanks for the patch, this is close. I like the direction it's going.

Just one more bigger thing, otherwise just nits:

In the case that you have argc, I think it might make sense to have a version of
StackOperandForArgument that isn't reversed, e.g. :

StackOperandForArgument(0, argc) would return the first argument.

You won't use this version when the offset is a constant, but whenever it's
already computed from argc, I think it makes it clearer.

https://codereview.chromium.org/21123008/diff/15001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21123008/diff/15001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4223: int extra_argument_offset = extra_stack_space /
kPointerSize;
Just make this 

int extra_argument_offset = HasCallSiteInlineCheck() ? 1 : 0

and get rid of extra_stack_space

https://codereview.chromium.org/21123008/diff/15001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4324: __ ret(2 * kPointerSize + extra_stack_space);
__ ret((extra_argument_offset + 2) * kPointerSize);

https://codereview.chromium.org/21123008/diff/15001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4346: __ ret(2 * kPointerSize + extra_stack_space);
__ ret((extra_argument_offset + 2) * kPointerSize);

https://codereview.chromium.org/21123008/diff/15001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/21123008/diff/15001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:417: for (int i = 1; i <= kFastApiCallArguments; i++)
{
Just change the loop to iterate from 0 to < kFastApiCallArguments, then you can
pass "i" instead of "i - 1"

[haitao.feng, 2013-08-02 15:04:58]

danno, thanks for the review. All comments addressed.

https://codereview.chromium.org/21123008/diff/24001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21123008/diff/24001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6027: __ movq(scratch, StackOperandForArgument(1, 2,
kPointerSize));
There is a push instruction between the function start and this argument access.
So I introduce the disp parameter in StackOperandFor[Argument|ReversedArgument].
The disp is 0 by default.

https://codereview.chromium.org/21123008/diff/24001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6047: __ cmpq(scratch, StackOperandForArgument(0, 2,
kPointerSize));
Same explanation above.

[haitao.feng, 2013-08-05 01:39:47]

danno, I am not satisfied with patch set 4 to address your comment for accessing
the first stack argument. So I worked out patchset 5 which introduces
StackArguments class (after declaration, we could access its elements like an
array, allowing negative index). 

From reading the current codes, it seems that we could use a descriptor (similar
like hydrogen stub descriptor) to initialize ParameterCount and StackArguments
to remove some boilerplate assembly codes. I leave it to future CL.

The StackArugments class is platform independent, currently is only used for
X64, maybe we could place it into code.h (ParameterCount lives there) or other
header file in the shared folder.

https://codereview.chromium.org/21123008/diff/28001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21123008/diff/28001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6039: StackArguments stack_arguments(2, 0,
kPointerSize);
There is a push instruction between the function start and the following
arguments access. So the disp is kPointerSize here. The disp is 0 for all the
other places.

[danno, 2013-08-05 15:17:56]

I generally like the direction this is going. However one high-level
observation: now that you've so nicely wrapped argument access, I wonder why
this is only x64 (seems to make exactly as much sense for at least ia32,
probably ARM/MIPS, too).

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1482: class StackArguments BASE_EMBEDDED {
I would call this StackArgumentsAccessor

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1490: StackArguments(Register reg, int extra = 0,
int disp = 0)
I find all the math with extra and disp a bit confusing. How about adding an
enum for the receiver mode:

StackArgumentsAccessorReceiverMode {
  ARGUMENTS_CONTAIN_RECEIVER,
  ARGUMENTS_DONT_CONTAIN_RECEIVER
};

The default should be ARGUMENTS_CONTAIN_RECEIVER since you more often than not
pass 1 for extra currently. You can then turn this into receiver_size_ that is
used in the math calculations below.

Also, call disp and disp_ dispacement_to_sp and dispacement_to_sp_,
respectively, this make the comment superfluous.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1502: Operand operator[] (int index) {
I think it's better to not overide the [] operator, since StackArgumentsAccessor
isn't a container per se. I think it would be better to call this 

GetArgumentOperand(int index) { ... }

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1516: } else {
I think the handling of negative arguments is pretty confusing. Do you really
need to support them?

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1523: Operand GetReceiver() {
... and this GetReceiverOperand()

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1580: // Computes the argument address from the
argument 0 (might be receiver)
Do you need any of these here and below any more?

https://codereview.chromium.org/21123008/diff/28001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/21123008/diff/28001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:417: StackArguments
stack_arguments(kFastApiCallArguments);
Here and elsewhere, how about just using the variable name "args"? It's shorter,
and leaves room for a longer, more descriptive accessor method name.

https://codereview.chromium.org/21123008/diff/28001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:471: __ movq(stack_arguments[-2], rdi);
Again, is it possible to avoid the negative arguments?

[haitao.feng, 2013-08-06 11:49:55]

danno, thanks a lot for the review. I tried to add StackArgumentsAccessor in the
code.h, there are two issues to address:
  1) abstract stack_pointer, it seems that all the platforms have their own
rsp/esp/sp pointer, this issue is easy to solve.
  2) abstract two addressing modes, "sp + reg*scale + disp" and "sp + disp".
Currently Operand class is defined in the different platforms and it seems that
ARM and MIPS does not support sp + reg*scale + disp, this issue is hard.  

So I still put StackArgumentsAccess in the X64 macro assembler and all the other
comments are addressed.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1482: class StackArguments BASE_EMBEDDED {
On 2013/08/05 15:17:56, danno wrote:
> I would call this StackArgumentsAccessor

Done.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1490: StackArguments(Register reg, int extra = 0,
int disp = 0)
On 2013/08/05 15:17:56, danno wrote:
> I find all the math with extra and disp a bit confusing. How about adding an
> enum for the receiver mode:
> 
> StackArgumentsAccessorReceiverMode {
>   ARGUMENTS_CONTAIN_RECEIVER,
>   ARGUMENTS_DONT_CONTAIN_RECEIVER
> };
> 
> The default should be ARGUMENTS_CONTAIN_RECEIVER since you more often than not
> pass 1 for extra currently. You can then turn this into receiver_size_ that is
> used in the math calculations below.
> 
> Also, call disp and disp_ dispacement_to_sp and dispacement_to_sp_,
> respectively, this make the comment superfluous.

Done.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1502: Operand operator[] (int index) {
On 2013/08/05 15:17:56, danno wrote:
> I think it's better to not overide the [] operator, since
StackArgumentsAccessor
> isn't a container per se. I think it would be better to call this 
> 
> GetArgumentOperand(int index) { ... } 

Done.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1516: } else {
On 2013/08/05 15:17:56, danno wrote:
> I think the handling of negative arguments is pretty confusing. Do you really
> need to support them?

Done.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1523: Operand GetReceiver() {
On 2013/08/05 15:17:56, danno wrote:
> ... and this GetReceiverOperand()

Done.

https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:1580: // Computes the argument address from the
argument 0 (might be receiver)
Done. Sorry, I remembered I removed all of them. Maybe I did not commit it.

https://codereview.chromium.org/21123008/diff/28001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

https://codereview.chromium.org/21123008/diff/28001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:417: StackArguments
stack_arguments(kFastApiCallArguments);
On 2013/08/05 15:17:56, danno wrote:
> Here and elsewhere, how about just using the variable name "args"? It's
shorter,
> and leaves room for a longer, more descriptive accessor method name.

Done.

https://codereview.chromium.org/21123008/diff/28001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:471: __ movq(stack_arguments[-2], rdi);
Done. Compute from argument[0] by using argc.

[haitao.feng, 2013-08-07 08:59:08]

high-level feedback is also addressed. The class StackArgumentsAccessor is moved
into code.h and a new file code-x64.cc is added. No impact to the other
platforms. This CL only contains the base register is rsp, rbp will be used for
some places in https://codereview.chromium.org/22267005/.

[danno, 2013-08-07 19:19:06]

Thanks for the patch, I really like the direction that it's going and I think
it's almost there.

I would prefer not adding the StackArgumentsAccessor to code.h and adding the
new code-x64.cc file. Simply add the declarations and definitions to the
codegen-x64.h/cc, that's where we've traditionally put stuff like this. If we
ever use this for ia32, we can simply duplicate the code (as we have done many
other times), or somehow figure out how to share code between Intel
architectures, but that's for another CL sometime in the distant future.

[haitao.feng, 2013-08-08 07:35:06]

danno, thanks for the review. I am glad you like this direction. The comment is
addressed. I will work on https://codereview.chromium.org/22267005/ to use the
StackArgumentsAccessor interface after this CL is landed.

[danno, 2013-08-13 09:35:09]

lgtm if you address the nits, but please wait to land until we've opened the
tree again after this week's stability.

https://codereview.chromium.org/21123008/diff/49001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21123008/diff/49001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:2464: 
nit: please remove unrelated whitespace change.

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h
File src/x64/codegen-x64.h (right):

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h#new...
src/x64/codegen-x64.h:119: ARGUMENTS_CONTAIN_RECEIVER,
nit: four space indent from previous line.

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h#new...
src/x64/codegen-x64.h:132: ARGUMENTS_CONTAIN_RECEIVER,
nit: four space indent from previous line.

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h#new...
src/x64/codegen-x64.h:146: int extra_displacement_to_last_argument = 0)
nit: four space indent from previous line.

[haitao.feng, 2013-08-13 11:13:50]

danno, thanks for the review. I will wait and commit this when the tree is open.

https://codereview.chromium.org/21123008/diff/49001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21123008/diff/49001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:2464: 
On 2013/08/13 09:35:09, danno wrote:
> nit: please remove unrelated whitespace change.

Done.

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h
File src/x64/codegen-x64.h (right):

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h#new...
src/x64/codegen-x64.h:119: ARGUMENTS_CONTAIN_RECEIVER,
On 2013/08/13 09:35:09, danno wrote:
> nit: four space indent from previous line.

Done.

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h#new...
src/x64/codegen-x64.h:132: ARGUMENTS_CONTAIN_RECEIVER,
On 2013/08/13 09:35:09, danno wrote:
> nit: four space indent from previous line.

Done.

https://codereview.chromium.org/21123008/diff/49001/src/x64/codegen-x64.h#new...
src/x64/codegen-x64.h:146: int extra_displacement_to_last_argument = 0)
On 2013/08/13 09:35:09, danno wrote:
> nit: four space indent from previous line.

Done.

[haitao.feng, 2013-08-27 01:21:55]

Committed patchset #11 manually as r16345 (presubmit successful).