X64: JSEntry Stub

X64: JSEntry Stub

[William Hesse, 2009-06-04 09:51:10]

Looks great.  Can we test it somehow?

http://codereview.chromium.org/118115/diff/9/1009
File src/x64/assembler-x64.cc (left):

http://codereview.chromium.org/118115/diff/9/1009#oldcode488
Line 488: 
Leave two lines between (non-inline) function definitions.

http://codereview.chromium.org/118115/diff/9/1009
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1009#newcode498
Line 498: void Assembler::call(const Operand& op) {
Not in alphabetical order.

http://codereview.chromium.org/118115/diff/9/1009#newcode698
Line 698: void Assembler::load_rax(void* value, RelocInfo::Mode mode) {
Not in alphabetical order.

http://codereview.chromium.org/118115/diff/9/1009#newcode703
Line 703: emitq(reinterpret_cast<uintptr_t>(value), mode);
Should we have an emitq that takes a pointer?  Should we always be using Address
(= byte* typedef) rather than void*?  Use Address if you can.

http://codereview.chromium.org/118115/diff/9/1009#newcode818
Line 818: void Assembler::movq(Register dst, void* value, RelocInfo::Mode rmode)
{
Can we use Address rather than void*?

http://codereview.chromium.org/118115/diff/9/1009#newcode1116
Line 1116: void Assembler::store_rax(void* dst, RelocInfo::Mode mode) {
Ditto.

http://codereview.chromium.org/118115/diff/9/1011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1011#newcode241
Line 241: __ movq(rbp, rsp);
The x64 compilers seem to compile without frame pointers, and so the debugger
doesn't expect them.  Unless we use them, we should perhaps not create them.
Look into what to do to make the debugger read this frame correctly as a C
frame.  I think doing this makes the debugger read the previous frame at an
off-by-one address.

http://codereview.chromium.org/118115/diff/9/1011#newcode246
Line 246: __ push(Immediate(~ArgumentsAdaptorFrame::SENTINEL));
This seems really weird, to use ~x just to get something that is not x.  I think
ArgumentsAdaptorFrame should supply NON_SENTINEL, or something similar, unless
there is a good motivation for doing this.  This is a minor point, I guess, but
if ~ is needed, comment it.

http://codereview.chromium.org/118115/diff/9/1011#newcode276
Line 276: __ push(rax);  // flush TOS
This comment is not clear to me.  How does pushing something flush something? 
What is in rax?

http://codereview.chromium.org/118115/diff/9/1011#newcode288
Line 288: // stub, because the builtin stubs may not have been generated yet.
I'm not sure this comment needs to be here, because you are almost as efficient
loading the reference indirectly, and you can nicely indirect to the right field
in the call operand.

http://codereview.chromium.org/118115/diff/9/1012
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/118115/diff/9/1012#newcode45
Line 45: static const int kPPOffset    = 1 * kPointerSize;
Should we write all of these as
kPPOffset = kNextOffset + kPointerSize; ?
That seems to be what is done in the rest of the code.

http://codereview.chromium.org/118115/diff/9/1013
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1013#newcode76
Line 76: ASSERT(StackHandlerConstants::kSize == 6 * kPointerSize);  // adjust
this code
Can you use a defined constant or the difference between two, instead of 6 *
kPointerSize?

http://codereview.chromium.org/118115/diff/9/1013#newcode99
Line 99: // Cached TOS.
What does this mean?

http://codereview.chromium.org/118115/diff/9/1013#newcode100
Line 100: movq(rax, Operand(kScratchRegister, 0));
Isn't this two-instruction sequence the same as loadq?

http://codereview.chromium.org/118115/diff/9/1013#newcode102
Line 102: movq(Operand(kScratchRegister, 0), rsp);
Ok, is this why you don't use loadq and storeq?  Because the address is
repeated?

[Lasse Reichstein, 2009-06-04 11:45:57]

http://codereview.chromium.org/118115/diff/9/1009
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1009#newcode703
Line 703: emitq(reinterpret_cast<uintptr_t>(value), mode);
Agree on using Address if possible.

http://codereview.chromium.org/118115/diff/9/1011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1011#newcode241
Line 241: __ movq(rbp, rsp);
We do use the frame pointers in the JS frames, e.g., for traversing the JS stack
during GC.

http://codereview.chromium.org/118115/diff/9/1011#newcode246
Line 246: __ push(Immediate(~ArgumentsAdaptorFrame::SENTINEL));
Agree.

http://codereview.chromium.org/118115/diff/9/1011#newcode276
Line 276: __ push(rax);  // flush TOS
Comment isn't clear to me either. Copied verbatim from ia32 code.
Rax contains the value of the c entry frame pointer (loaded using
load_rax(c_entry_fp) above.

http://codereview.chromium.org/118115/diff/9/1011#newcode288
Line 288: // stub, because the builtin stubs may not have been generated yet.
We could just as easily do a movq(rax, some-code-address, CODE_TARGET) in the
two branches, if it wasn't because the code we refer to might not exist yet. 
I reworded the comment to make it more explicit.

http://codereview.chromium.org/118115/diff/9/1012
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/118115/diff/9/1012#newcode45
Line 45: static const int kPPOffset    = 1 * kPointerSize;
Would be consistent, but you can't do that with static fields, only with enums.

http://codereview.chromium.org/118115/diff/9/1013
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1013#newcode76
Line 76: ASSERT(StackHandlerConstants::kSize == 6 * kPointerSize);  // adjust
this code
Doesn't really make sense to introduce a constant and only use it in an assert.
It could do with a comment, though.

http://codereview.chromium.org/118115/diff/9/1013#newcode99
Line 99: // Cached TOS.
I'm assuming the content of Top::k_handler is also called TOS, which I also
assume means Top Of Stack.
Since two people can read the comment and not get any wiser, I guess the comment
is insufficiently detailed :)

http://codereview.chromium.org/118115/diff/9/1013#newcode102
Line 102: movq(Operand(kScratchRegister, 0), rsp);
Exactly. I need it in a register for the store (because the value is not in
rax), so this way is shorter than duplicating the address in the load_rax.

[Kevin Millikin (Chromium), 2009-06-04 11:53:44]

http://codereview.chromium.org/118115/diff/9/1011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1011#newcode276
Line 276: __ push(rax);  // flush TOS
On 2009/06/04 11:45:57, Lasse Reichstein wrote:
> Comment isn't clear to me either. Copied verbatim from ia32 code.
> Rax contains the value of the c entry frame pointer (loaded using
> load_rax(c_entry_fp) above.

No, it contains the top handler presumably left in rax by PushTryHandler (if the
handler block layout is the same as in IA32).

This is a holdover from top-of-stack caching, but there is no good reason not to
push the entire handler block in PushTryHandler.

[Kevin Millikin (Chromium), 2009-06-04 15:05:29]

A few more drive-by comments.  The biggest thing is that I think you should try
to do the right thing (in general with x64) rather than copying the other
platforms.

The exception handler has a couple of fields that (to my knowledge) are
completely unused.  They haven't been removed from IA32 because there are some
annoying assumptions about handler height etc.

So: if they're a little tricky to pull out of IA32 or ARM, I would avoid
introducing them to a new port in the first place.  A good strategy might be to
*only* introduce what you actually need (handler address and link to the
previous right now)....

http://codereview.chromium.org/118115/diff/9/1011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1011#newcode53
Line 53: #define __ masm->
You should use ACCESS_MASM from macro-assembler-x64.h to get code coverage.  Per
the style guide you should define this immediately before the first function
that uses it and undefine it afterward.

http://codereview.chromium.org/118115/diff/9/1011#newcode254
Line 254: __ push(rbx);
I have a feeling that the saving and restoring of callee-saved registers should
just be abstracted into a macro-assembler function.

http://codereview.chromium.org/118115/diff/9/1012
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/118115/diff/9/1012#newcode45
Line 45: static const int kPPOffset    = 1 * kPointerSize;
This is not PP (there is not PP).  In fact, I'm nearly positive you can just
leave it out of the handler.

http://codereview.chromium.org/118115/diff/9/1012#newcode48
Line 48: static const int kCodeOffset  = 3 * kPointerSize;
This field is also unused, no?

[Lasse Reichstein, 2009-06-09 12:01:43]

http://codereview.chromium.org/118115/diff/9/1009
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/118115/diff/9/1009#newcode703
Line 703: emitq(reinterpret_cast<uintptr_t>(value), mode);
I think it's better to keep it at void*. There isn't any simple way to convert A
Smi* or Failure* to an Address (since they aren't really adresses), but we do
need to load them anyway.