Bug 123806 – Get rid of the regT* definitions in JSInterfaceJIT.h

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

[Mark Lam]

We now have regT* definitions in GPRInfo.h and JSInterfaceJIT.h, and the 2 can get out of sync.  This results in subtle bugs that are hard to detect.  We're going to make GPRInfo.h the definitive authority on regT* definitions and remove the redundant definitions in JSInterfaceJIT.h.

[Mark Lam]

Created attachment 216352 [details]
the patch.

There will be 1 style failure expected.

[Mark Lam]

Comment on attachment 216352 [details]
the patch.

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

> Source/JavaScriptCore/ChangeLog:8
> +        JSInterfaceJIT now inherits on GPRInfo and FPRInfo, and relies on them

"inherits on" ==> "inherits from".  Will fix.

[WebKit Commit Bot]

Attachment 216352 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/jit/GPRInfo.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITArithmetic.cpp', u'Source/JavaScriptCore/jit/JITArithmetic32_64.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JSInterfaceJIT.h', u'Source/JavaScriptCore/jit/SlowPathCall.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JSInterfaceJIT.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 216353 [details]
patch 2.

[WebKit Commit Bot]

Attachment 216353 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/jit/GPRInfo.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITArithmetic.cpp', u'Source/JavaScriptCore/jit/JITArithmetic32_64.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JSInterfaceJIT.h', u'Source/JavaScriptCore/jit/SlowPathCall.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JSInterfaceJIT.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 216353 [details]
patch 2.

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.

[Mark Lam]

(In reply to comment #6)
> > 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.

Thanks.  I shouldn't have propagated that comment block without digesting it first.  I've reduced the comment preceding the GPRInfo definitions to:

// The baseline JIT requires that regT3 be callee-preserved.

And added the following after the GPRInfo definitions:

// The baseline JIT uses "accumulator" style execution with regT0 (for 64-bit)
// and regT0 + regT1 (for 32-bit) serving as the accumulator register(s) for
// passing results of one opcode to the next. Hence:
COMPILE_ASSERT(GPRInfo::regT0 == GPRInfo::returnValueGPR, regT0_must_equal_returnValueGPR);
#if USE(JSVALUE32_64)
COMPILE_ASSERT(GPRInfo::regT1 == GPRInfo::returnValueGPR2, regT1_must_equal_returnValueGPR2);
#endif

[Mark Lam]

Thanks for the review.  Landed in r158901: <http://trac.webkit.org/r158901>.