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-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 {
I would call this StackArgumentsAccessor
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)
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-x64.h#newcode1502
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-x64.h#newcode1516
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-x64.h#newcode1523
src/x64/macro-assembler-x64.h:1523: Operand GetReceiver() {
... and this GetReceiverOperand()
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)
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#newcode417
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#newcode471
src/x64/stub-cache-x64.cc:471: __ movq(stack_arguments[-2], rdi);
Again, is it possible to avoid the negative arguments?
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.
