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-x64.h
File src/x64/macro-assembler-x64.h (right):
https://codereview.chromium.org/21123008/diff/28001/src/x64/macro-assembler-x64.h#newcode1482
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-x64.h#newcode1490
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-x64.h#newcode1502
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-x64.h#newcode1516
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-x64.h#newcode1523
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-x64.h#newcode1580
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#newcode417
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#newcode471
src/x64/stub-cache-x64.cc:471: __ movq(stack_arguments[-2], rdi);
Done. Compute from argument[0] by using argc.
https://codereview.chromium.org/21123008/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
