Use StackArgumenstAccessor and kPCOnStackSize/kFPOnStackSize to compute stack address/operand

Use StackArgumenstAccessor and kPCOnStackSize/kFPOnStackSize to compute stack address/operand for X64

R=danno@chromium.org

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

[haitao.feng, 2013-08-06 13:43:55]

Those computation works for X32 also.

[haitao.feng, 2013-08-07 08:22:56]

I realized I could merge this CL with https://codereview.chromium.org/21123008/.
Please ignore it at this moment. They are not in
https://codereview.chromium.org/21123008/ as either we access the argument from
rbp or the original code does not use explicit argument address calculation.

[haitao.feng, 2013-08-27 13:33:00]

Rebased with master. Please take a look.

[haitao.feng, 2013-09-10 05:53:36]

Rebased with master and refactored newly-introduced GenerateFastApiCall in
stub-cache-x64.cc with StackArgumentsAccessor.

[danno, 2013-09-18 12:08:42]

Thanks for the patch. I like this cleanup a lot, perhaps you can just go a
little further (see comments).

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

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:2396: StackArgumentsAccessor args(rsp, 4,
ARGUMENTS_DONT_CONTAIN_RECEIVER);
Since you are going through all the trouble to clean this up, you also please
turn the hard coded numbers into constants?

enum RegExpExecStubArgumentIndices {
  JS_REG_EXP_OBJECT_ARGUMENT_INDEX,
  SUBJECT_STRING_ARGUMENT_INDEX,
  PREVIOUS_INDEX_ARGUMENT_INDEX,
  LAST_MATCH_INFO_ARGUMENT_INDEX,
  REG_EXP_EXEC_ARGUMENT_COUNT 
} 

And use them where appropriate?

This also applies to the changes later in this file.

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:2397: Operand StackOperandForJSRegExpObject =
args.GetArgumentOperand(0);
nit: these variable names should start with a lower case letter.

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4951: StackArgumentsAccessor args(rsp, 3,
ARGUMENTS_DONT_CONTAIN_RECEIVER);
See comment above about using constants.

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4978: __ ret(3 * kPointerSize);
You can use the argument count instead of a magic number here and below of if
you define the enum.

https://codereview.chromium.org/22267005/diff/12001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/22267005/diff/12001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:2653: STATIC_ASSERT(StackHandlerConstants::kSize
>= 5 * kPointerSize);
Why not (StackHandlerConstants::kSiz == 4 * kPointerSize + kFPSizeOnStack)? Or
do I misunderstand something? The >= comparison generally seems a lot weaker
(same below)

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

https://codereview.chromium.org/22267005/diff/12001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:2336: __ movq(rax, args.GetArgumentOperand(argc));
1 instead of argc

[haitao.feng, 2013-09-22 08:16:00]

danno, thanks for the review. All the comments are addressed. 

I will rebase with master to handle https://codereview.chromium.org/23903053 in
this CL.

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

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:2396: StackArgumentsAccessor args(rsp, 4,
ARGUMENTS_DONT_CONTAIN_RECEIVER);
On 2013/09/18 12:08:42, danno wrote:
> Since you are going through all the trouble to clean this up, you also please
> turn the hard coded numbers into constants?
> 
> enum RegExpExecStubArgumentIndices {
>   JS_REG_EXP_OBJECT_ARGUMENT_INDEX,
>   SUBJECT_STRING_ARGUMENT_INDEX,
>   PREVIOUS_INDEX_ARGUMENT_INDEX,
>   LAST_MATCH_INFO_ARGUMENT_INDEX,
>   REG_EXP_EXEC_ARGUMENT_COUNT 
> } 
> 
> And use them where appropriate?
> 
> This also applies to the changes later in this file.

Done.

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:2397: Operand StackOperandForJSRegExpObject =
args.GetArgumentOperand(0);
Removed these variables.
On 2013/09/18 12:08:42, danno wrote:
> nit: these variable names should start with a lower case letter.

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4951: StackArgumentsAccessor args(rsp, 3,
ARGUMENTS_DONT_CONTAIN_RECEIVER);
On 2013/09/18 12:08:42, danno wrote:
> See comment above about using constants.

Done.

https://codereview.chromium.org/22267005/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4978: __ ret(3 * kPointerSize);
On 2013/09/18 12:08:42, danno wrote:
> You can use the argument count instead of a magic number here and below of if
> you define the enum.

Done.

https://codereview.chromium.org/22267005/diff/12001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/22267005/diff/12001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:2653: STATIC_ASSERT(StackHandlerConstants::kSize
>= 5 * kPointerSize);
On 2013/09/18 12:08:42, danno wrote:
> Why not (StackHandlerConstants::kSiz == 4 * kPointerSize + kFPSizeOnStack)? Or
> do I misunderstand something? The >= comparison generally seems a lot weaker
> (same below)

Done.

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

https://codereview.chromium.org/22267005/diff/12001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:2336: __ movq(rax, args.GetArgumentOperand(argc));
On 2013/09/18 12:08:42, danno wrote:
> 1 instead of argc

Done.

[haitao.feng, 2013-09-22 09:44:28]

I rebased with master and marked the changes needing review between patchset 4
and patchset 5. I verified X32 port could run successfully with the changes
between them.

https://codereview.chromium.org/22267005/diff/24001/src/arguments.h
File src/arguments.h (right):

https://codereview.chromium.org/22267005/diff/24001/src/arguments.h#newcode241
src/arguments.h:241: static const int kContextSaveIndex = T::kContextSaveIndex;
This change needs review.

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

https://codereview.chromium.org/22267005/diff/24001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:527: StackArgumentsAccessor args_from_rbp(rbp,
kFastApiCallArguments,
Line 527 to 538 needs review.

https://codereview.chromium.org/22267005/diff/24001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:1115: StackArgumentsAccessor args(rsp,
kFastApiCallArguments,
This needs review.

https://codereview.chromium.org/22267005/diff/24001/src/x64/stub-cache-x64.cc...
src/x64/stub-cache-x64.cc:1398: // The name handler is counted as an argument.
Line 1398 to 1407 need review.

[danno, 2013-09-28 07:43:51]

lgtm

[haitao.feng, 2013-09-30 14:12:47]

danno, thanks for the review during weekends.

I will wait marja to commit https://codereview.chromium.org/24488006/, and then
upload a CL to rebase "Refactor PropertyCallbackInfo & FunctionCallbackInfo,
part 2 and part 3" (I need to make sure it works for x32.release.check), and
then rebase and commit this CL.

[haitao.feng, 2013-10-03 01:22:46]

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