Bug 123956 – Change CallFrameRegister to architected frame pointer register

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

This is part of moving to the C++ stack.

[Michael Saboff]

Created attachment 216260 [details]
Patch

[Filip Pizlo]

Comment on attachment 216260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216260&action=review

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:166
> +    if (MacroAssembler::framePointerRegister == GPRInfo::callFrameRegister)
> +        jit.pop(GPRInfo::regT3); // ignore prior framePointer
> +    else
> +        jit.pop(MacroAssembler::framePointerRegister);

Why is this an 'if' statement?  Isn't it always true after your patch?

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 216260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216260&action=review
> 
> > Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:166
> > +    if (MacroAssembler::framePointerRegister == GPRInfo::callFrameRegister)
> > +        jit.pop(GPRInfo::regT3); // ignore prior framePointer
> > +    else
> > +        jit.pop(MacroAssembler::framePointerRegister);
> 
> Why is this an 'if' statement?  Isn't it always true after your patch?

It isn't needed, but I thought it would be helpful for debugging if one wants to change the register assignments.  Also, MacroAssembler::framePointerRegister and GPRInfo::callFrameRegister are different names.  If you like, I can change this to a RELEASE_ASSERT.

[Geoffrey Garen]

> If you like, I can change this to a RELEASE_ASSERT.

static_assert is even better for something like this, since the two sides are both constants.

[Geoffrey Garen]

Comment on attachment 216260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216260&action=review

You mentioned that this patch broke the debugger in some way. Did you get to the bottom of that? Do we need a bug about that?

r=me, with some fixups below.

> Source/JavaScriptCore/ChangeLog:8
> +        Changed X86 and ARM variants as well as MIPS to use their respective arcitected

Should be "architected" or "architectures'".

> Source/JavaScriptCore/ChangeLog:9
> +        frame pointer register.  For X86, this freed up another register as a temp or for

Should be "frame pointer registers".

As a temp for what? Baseline JIT? Did you start using that register somewhere?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:80
> +    // Restore the callFrame value into a temp, as the framePointerRegister may be the callFrameRegister
> +    // Below we use the temp register when accessing the destination call frame.

There are two problems with this comment.

(1) You say "may be" when you mean "I have been working for months to ensure that this is definitely so, and many levels of our architecture rely on it being so". When you say "may" it implies that sometimes this many not be the case, and it's the reader's job to worry about the times when it's not the case. Very confusing.

(2) Your comment explains why you changed from the old code to the new code, and it makes sense in the context of a diff that shows you removing the old code that used callFrameRegister. However, it won't make any sense to a future programmer, who only sees the new code.

I think this comment should just say "regT4 points to the exiting function's call frame".

>>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:166
>>> +        jit.pop(MacroAssembler::framePointerRegister);
>> 
>> Why is this an 'if' statement?  Isn't it always true after your patch?
> 
> It isn't needed, but I thought it would be helpful for debugging if one wants to change the register assignments.  Also, MacroAssembler::framePointerRegister and GPRInfo::callFrameRegister are different names.  If you like, I can change this to a RELEASE_ASSERT.

static_assert, please.

[Michael Saboff]

(In reply to comment #5)
> (From update of attachment 216260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216260&action=review
> 
> You mentioned that this patch broke the debugger in some way. Did you get to the bottom of that? Do we need a bug about that?

lldb won't display registers via "reg read" in some cases after this change.  I'll create a test case and file a bug.

> r=me, with some fixups below.
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Changed X86 and ARM variants as well as MIPS to use their respective arcitected
> 
> Should be "architected" or "architectures'".

Fixed.

> > Source/JavaScriptCore/ChangeLog:9
> > +        frame pointer register.  For X86, this freed up another register as a temp or for
> 
> Should be "frame pointer registers".

Fixed.
 
> As a temp for what? Baseline JIT? Did you start using that register somewhere?

Actually I added a new temp and made the register available to the DFG allocator.  I updated the code since this review to add back all the JSC specific callFrame registers made available with the change.  I updated the comment accordingly.
 
> > Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:80
> > +    // Restore the callFrame value into a temp, as the framePointerRegister may be the callFrameRegister
> > +    // Below we use the temp register when accessing the destination call frame.
> 
> There are two problems with this comment.
> 
> (1) You say "may be" when you mean "I have been working for months to ensure that this is definitely so, and many levels of our architecture rely on it being so". When you say "may" it implies that sometimes this many not be the case, and it's the reader's job to worry about the times when it's not the case. Very confusing.
> 
> (2) Your comment explains why you changed from the old code to the new code, and it makes sense in the context of a diff that shows you removing the old code that used callFrameRegister. However, it won't make any sense to a future programmer, who only sees the new code.
> 
> I think this comment should just say "regT4 points to the exiting function's call frame".

I changed it to: "Restore the exiting function's callFrame value into a regT4".

> >>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:166
> >>> +        jit.pop(MacroAssembler::framePointerRegister);
> >> 
> >> Why is this an 'if' statement?  Isn't it always true after your patch?
> > 
> > It isn't needed, but I thought it would be helpful for debugging if one wants to change the register assignments.  Also, MacroAssembler::framePointerRegister and GPRInfo::callFrameRegister are different names.  If you like, I can change this to a RELEASE_ASSERT.
> 
> static_assert, please.

Done.

[Michael Saboff]

Committed r158883: <http://trac.webkit.org/changeset/158883>