[webkit-reviews] review granted: [Bug 123806] Get rid of the regT* definitions in JSInterfaceJIT.h : [Attachment 216353] patch 2.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 123806: Get rid of the regT* definitions in JSInterfaceJIT.h
https://bugs.webkit.org/show_bug.cgi?id=123806

Attachment 216353: patch 2.
https://bugs.webkit.org/attachment.cgi?id=216353&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216353&action=review


r=me

> Source/JavaScriptCore/jit/GPRInfo.h:286
> +// regT0 has two special meanings. The return value from a stub
> +// call will always be in regT0, and by default (unless
> +// a register is specified) emitPutVirtualRegister() will store
> +// the value from regT0.

I know that you're just moving a pre-existing comment, but since you champion
comments so strongly, I'm sure it bothers you that this comment is sub-par, and
you want to improve it. So, I'll tell you how to.

First, you can improve the comment about return values by changing it to a
static_assert that regT0 is equal to returnValueRegister. Assert is a much more
reliable way to indicate an assertion of fact.

Second, you can remove the comment about emitPutVirtualRegister. GPRInfo has no
business making promises about how emitPutVirtualRegister(), a client function,
will work. emitPutVirtualRegister() comments itself by specifying a default
argument value, and it is free to specify whatever value it likes.

Third, the static_assert is also true of regT0+regT1 on 32bit. So, you should
add a static_assert for that, too.

Fourth, regT0 on 64bit, and regT0+regT1 on 32bit, are used as an "accumulator"
style of register in the baseline JIT, caching the value of the last opcode, so
that it doesn't need to be reloaded from the stack when reused by the next
opcode. That's a good comment to put into the baseline JIT. You can put the
static_assert there, too: the only reason we care about regT0 being equal to
the return value register (or, on 32bit, regT0+regT1 equal to the return value
registers) is that it facilities the accumulator style.

> Source/JavaScriptCore/jit/GPRInfo.h:293
> +// tempRegister2 is has no such dependencies. It is important that
> +// on x86/x86-64 it is ecx for performance reasons, since the
> +// MacroAssembler will need to plant register swaps if it is not -
> +// however the code will still function correctly.

You'll notice that tempRegister2 doesn't exist. So, this comment should be
removed.

I hope you didn't read this comment and take it to heart. If so, please go back
in your memory and erase its lesson, since what it told you was false.