[webkit-reviews] review granted: [Bug 123956] Change CallFrameRegister to architected frame pointer register : [Attachment 216260] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 123956: Change CallFrameRegister to architected frame pointer register
https://bugs.webkit.org/show_bug.cgi?id=123956

Attachment 216260: Patch
https://bugs.webkit.org/attachment.cgi?id=216260&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.