X64: Implement CEntryStub and JSEntryTrampoline.

X64: Implement CEntryStub and JSEntryTrampoline.
Still some supporting functions missing.

[Lasse Reichstein, 2009-06-08 13:07:51]

Review please.
It's lots of machine code that cannot run yet. Should be simple :P

[Kevin Millikin (Chromium), 2009-06-08 13:34:00]

Drive-bys.

http://codereview.chromium.org/114085/diff/1/6
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/6#newcode79
Line 79: __ movq(rbx, Operand(rbp, 0));
Consider an Operand constructor that takes just the register, no 0.  That (its
absence) has been a source of ia32 bugs.

http://codereview.chromium.org/114085/diff/1/8
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/114085/diff/1/8#newcode38
Line 38: static const RegList kJSCallerSaved =
Implement Register::bit() and write:
rax.bit() | rbx.bit() | rcx.bit() | rdx.bit() | rdi.bit()

[William Hesse, 2009-06-08 14:03:46]

http://codereview.chromium.org/114085/diff/1/4
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/4#newcode76
Line 76: 
Move the other constructor of Operand to here, from -inl.h

http://codereview.chromium.org/114085/diff/1/4#newcode85
Line 85: set_modrm(0, rsp);
Make a note on set_modrm() that it does not reset a REX.B bit set by set_sib(),
so it can safely be used with rsp after set_sib.

http://codereview.chromium.org/114085/diff/1/4#newcode889
Line 889: ASSERT_EQ(RelocInfo::NONE, mode);
Can't ExternalReferences have a relocation mode?  What about all the other
non-heap object modes.  I guess we know that only Object* can be made handles.

http://codereview.chromium.org/114085/diff/1/6
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/6#newcode70
Line 70: // stack.
What about saving callee-save registers?

http://codereview.chromium.org/114085/diff/1/6#newcode117
Line 117: __ add(rcx, Immediate(1));
Why not inc(rcx)?

http://codereview.chromium.org/114085/diff/1/6#newcode143
Line 143: void Builtins::Generate_JSEntryTrampoline(MacroAssembler* masm) {
Does the code that calls this function create the shared code object, and
register it with the stub database?  This has to be made a stub, right?

http://codereview.chromium.org/114085/diff/1/7
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/7#newcode389
Line 389: __ pop(rdi);  // PP
Can you find every handler, and stop it from pushing PP?

http://codereview.chromium.org/114085/diff/1/7#newcode412
Line 412: StackFrame::EXIT_DEBUG :
Change these names from EXIT to CENTRY.  EXIT makes me think we are destroying a
stack frame, not creating a new one.

http://codereview.chromium.org/114085/diff/1/8
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/114085/diff/1/8#newcode65
Line 65: class EntryFrameConstants : public AllStatic {
JSEntryFrameConstants

http://codereview.chromium.org/114085/diff/1/8#newcode76
Line 76: class ExitFrameConstants : public AllStatic {
CEntryFrameConstants

http://codereview.chromium.org/114085/diff/1/8#newcode88
Line 88: class StandardFrameConstants : public AllStatic {
What is a Standard frame?

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

http://codereview.chromium.org/114085/diff/1/9#newcode50
Line 50: // Not implemented yet.
I will do these. (Check, Assert, printing error messages)

http://codereview.chromium.org/114085/diff/1/9#newcode160
Line 160: for (int i = kNumJSCallerSaved; --i >= 0;) {
I prefer (i=kNumJSCallerSaved -1; i >=0 ; --i).  It's not as tricky.

[Kevin Millikin (Chromium), 2009-06-09 08:23:25]

Small round of comments.  Generally: this code needs to be better commented (I
know that IA32 code isn't either, but if you've done the work to read and
understand it, you should preserve it here).

http://codereview.chromium.org/114085/diff/1/6
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/6#newcode59
Line 59: const Register kFirstParameter = rsi;
These seem to be unused?

http://codereview.chromium.org/114085/diff/1/6#newcode69
Line 69: // Parameters in rcx, rdx, r8, r9, [rsp+0x20], with spill space
allocated on
It's not clear what those parameters are.  This comment is one that actually
deserves to be more verbose.

// rcx: ignored?
// rdx: JS function?
// r8: receiver?
// r9: arg count?
// [rsp+0x20]: arg array?

The reader doesn't want to have to reverse engineer them.

http://codereview.chromium.org/114085/diff/1/6#newcode72
Line 72: __ movq(r11, rdx);
Not clear why rdx is saved in r11 (or why it's done so eagerly).  Can't it just
be left in rdx for the MSVC case (and put in rdx for the other)?

http://codereview.chromium.org/114085/diff/1/6#newcode74
Line 74: __ xor_(rsi, rsi);  // clear rsi
// Clear rsi.

But this is such an idiom that there is probably no line comment needed (given
the comment preceding the line).

http://codereview.chromium.org/114085/diff/1/6#newcode81
Line 81: // Get the function (rdx) from the frame and setup the context.
Comment seems off.

// Load the context register with the function's context.

http://codereview.chromium.org/114085/diff/1/6#newcode91
Line 91: #else  // __MSVC__
I actually prefer this comment to indicate what condition causes the else to be
hit:

#else  // !defined(__MSVC__)

http://codereview.chromium.org/114085/diff/1/6#newcode92
Line 92: // GCC arguemnts in rdi, rsi, rdx, rcx, r8.
arguemnts => arguments

Also, tell us what each of them is.

http://codereview.chromium.org/114085/diff/1/6#newcode109
Line 109: 
Also a comment here telling what is currently live in registers (and the same
with both C++ compilers).  r11: JS Function, rsi: context, rax: arg count, rbx:
arg array.

http://codereview.chromium.org/114085/diff/1/6#newcode112
Line 112: __ xor_(rcx, rcx);  // clear ecx
Comment is wrong.

http://codereview.chromium.org/114085/diff/1/7
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/7#newcode237
Line 237: __ movq(rcx, Operand(rdx, -1 * kPointerSize));  // get next in chain
There is a constant in StackHandlerConstants that you should use here
(kAddressDisplacement).

http://codereview.chromium.org/114085/diff/1/8
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/114085/diff/1/8#newcode38
Line 38: static const RegList kJSCallerSaved =
On 2009/06/08 13:34:00, Kevin Millikin wrote:
> Implement Register::bit() and write:
> rax.bit() | rbx.bit() | rcx.bit() | rdx.bit() | rdi.bit()

Maybe that doesn't work because of circular dependencies.  That would be
annoying.

[Lasse Reichstein, 2009-06-09 09:48:43]

Addressed review comments. Please re-review.

http://codereview.chromium.org/114085/diff/1/4
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/4#newcode76
Line 76: 
On 2009/06/08 14:03:46, William Hesse wrote:
> Move the other constructor of Operand to here, from -inl.h
> 

Done.

http://codereview.chromium.org/114085/diff/1/4#newcode889
Line 889: ASSERT_EQ(RelocInfo::NONE, mode);
Yes, only Object* can occur in handles, and a non-heap-object doesn't have a
memory location.

http://codereview.chromium.org/114085/diff/1/6
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/6#newcode59
Line 59: const Register kFirstParameter = rsi;
Correct. Well spotted.

http://codereview.chromium.org/114085/diff/1/6#newcode70
Line 70: // stack.
They are saved by the JSEntry stub, on the frame below the JSEntryTrampoline.

http://codereview.chromium.org/114085/diff/1/6#newcode72
Line 72: __ movq(r11, rdx);
It could (rdx was used below, but that was easily changed to use
kScratchRegister).
For the GCC version, we need to move the function pointer away from rsi before
using rdx, so another register would be better. Since the construct call expects
the function in rdi, we might as well put it there from the start.

http://codereview.chromium.org/114085/diff/1/6#newcode74
Line 74: __ xor_(rsi, rsi);  // clear rsi
Agree. Bad cut-n-paste.

http://codereview.chromium.org/114085/diff/1/6#newcode79
Line 79: __ movq(rbx, Operand(rbp, 0));
The Operand(reg) constructor has been removed in x64. If we add it again,
meaning the value at the address stored in the register, and not the direct
value of the register as in ia-32, it will be far too easy to confuse the two
and do the wrong thing.
Perhaps we should rename Operand to MemOperand, like on ARM, since that is what
it is in X64.

http://codereview.chromium.org/114085/diff/1/6#newcode81
Line 81: // Get the function (rdx) from the frame and setup the context.
Fixed.

http://codereview.chromium.org/114085/diff/1/6#newcode91
Line 91: #else  // __MSVC__
Agree, strongly. I'll stop doing it this way immediately.
What should it say at the #endif?

http://codereview.chromium.org/114085/diff/1/6#newcode117
Line 117: __ add(rcx, Immediate(1));
General distrust for inc and dec.
They don't update the flags the same way as add/sub, which can cause the
pipeline to stall. In this case it probably doesn't matter, since its not used
together with other arithmetic operations.

http://codereview.chromium.org/114085/diff/1/6#newcode143
Line 143: void Builtins::Generate_JSEntryTrampoline(MacroAssembler* masm) {
The entry trampolines are not stubs, but built-ins. The comment at the call
point (codegen-x64.cc, JSEntryStub::GenerateBody(...)) specifically states that
the tarmpoline code might not have been created at the time the JSEntry stub is
compiled.

http://codereview.chromium.org/114085/diff/1/7
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/114085/diff/1/7#newcode237
Line 237: __ movq(rcx, Operand(rdx, -1 * kPointerSize));  // get next in chain
Fixed, and initial ASSERT made more relevant.

http://codereview.chromium.org/114085/diff/1/7#newcode389
Line 389: __ pop(rdi);  // PP
Will do.
It should be easier to do this incrementally, as we convert code, than to do it
afterwards.

http://codereview.chromium.org/114085/diff/1/7#newcode412
Line 412: StackFrame::EXIT_DEBUG :
The would be a major change (if done right, i.e., renaming "EXIT" to "C_ENTRY"
everywhere), spanning all three architectures and requiring a lot of testing.
It's a good idea, but not for this CL.

http://codereview.chromium.org/114085/diff/1/8
File src/x64/frames-x64.h (right):

http://codereview.chromium.org/114085/diff/1/8#newcode38
Line 38: static const RegList kJSCallerSaved =
It *is* annoying.

http://codereview.chromium.org/114085/diff/1/8#newcode88
Line 88: class StandardFrameConstants : public AllStatic {
Just a superclass of different JavaScript frames.

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

http://codereview.chromium.org/114085/diff/1/9#newcode160
Line 160: for (int i = kNumJSCallerSaved; --i >= 0;) {
I concur.

[Kevin Millikin (Chromium), 2009-06-09 11:23:26]

I've only looked closely at the entry stub.  It LGTM.

[William Hesse, 2009-06-10 08:09:09]

LGTM.

http://codereview.chromium.org/114085/diff/2005/1008
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/114085/diff/2005/1008#newcode902
Line 902: ASSERT(!Heap::InNewSpace(*value));
Is this from the ia32 code?  I didn't know this.  It makes sense, though - we
can't fix those up on copying collections.

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

http://codereview.chromium.org/114085/diff/2005/1013#newcode48
Line 48: void MacroAssembler::Check(Condition cc, const char* message) {
We want this to be inline if the debug check is inside it, don't we?